Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Date:   Wed, 4 Jul 2018 17:31:07 +0100
From:   Okash Khawaja <osk@fb.com>
To:     Jakub Kicinski <jakub.kicinski@netronome.com>
CC:     Daniel Borkmann <daniel@iogearbox.net>,
        Martin KaFai Lau <kafai@fb.com>,
        Alexei Starovoitov <ast@kernel.org>,
        Yonghong Song <yhs@fb.com>,
        Quentin Monnet <quentin.monnet@netronome.com>,
        "David S. Miller" <davem@davemloft.net>, <netdev@vger.kernel.org>,
        <kernel-team@fb.com>, <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH bpf-next v2 2/3] bpf: btf: add btf print functionality
Message-ID: <20180704163106.GA2200@w1t1fb>
References: <20180702183913.669030439@fb.com>
 <20180702191324.570616684@fb.com>
 <20180702220659.6baa77ba@cakuba.netronome.com>
 <20180703214559.GA4448@w1t1fb>
 <20180703152331.151d1c4b@cakuba.netronome.com>
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Disposition: inline
In-Reply-To: <20180703152331.151d1c4b@cakuba.netronome.com>
User-Agent: Mutt/1.9.5 (2018-04-13)
Received-SPF: None (protection.outlook.com: fb.com does not designate
 permitted sender hosts)
SpamDiagnosticOutput: 1:99
SpamDiagnosticMetadata: NSPM
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 04 Jul 2018 16:31:19.0886 (UTC)
X-MS-Exchange-CrossTenant-Network-Message-Id: 767d4175-5fdf-48b1-c047-08d5e1cb9313
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 8ae927fe-1255-47a7-a2af-5f3a069daaa2
X-MS-Exchange-Transport-CrossTenantHeadersStamped: SN6PR15MB2511
X-OriginatorOrg: fb.com
X-Proofpoint-Virus-Version: vendor=fsecure engine=2.50.10434:,, definitions=2018-07-04_06:,,
 signatures=0
X-Proofpoint-Spam-Reason: safe
X-FB-Internal: Safe
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

hi,

On Tue, Jul 03, 2018 at 03:23:31PM -0700, Jakub Kicinski wrote:
> On Tue, 3 Jul 2018 22:46:00 +0100, Okash Khawaja wrote:
> > On Mon, Jul 02, 2018 at 10:06:59PM -0700, Jakub Kicinski wrote:
> > > On Mon, 2 Jul 2018 11:39:15 -0700, Okash Khawaja wrote:  
> > > > +#define BITS_PER_BYTE_MASK (BITS_PER_BYTE - 1)
> > > > +#define BITS_PER_BYTE_MASKED(bits) ((bits) & BITS_PER_BYTE_MASK)  
> > > 
> > > Perhaps it's just me but BIT_OFFSET or BIT_COUNT as a name of this macro
> > > would make it more obvious to parse in the code below.  
> > I don't mind either. However these macro names are also used inside
> > kernel for same purpose. For sake of consistency, I'd recommend we keep
> > them :)
> 
> Ugh, okay :)
> 
> > > > +	} print_num;
> > > > +
> > > > +	total_bits_offset = bit_offset + BTF_INT_OFFSET(int_type);
> > > > +	data += BITS_ROUNDDOWN_BYTES(total_bits_offset);
> > > > +	bit_offset = BITS_PER_BYTE_MASKED(total_bits_offset);
> > > > +	bits_to_copy = bits + bit_offset;
> > > > +	bytes_to_copy = BITS_ROUNDUP_BYTES(bits_to_copy);
> > > > +
> > > > +	print_num.u64_num = 0;
> > > > +	memcpy(&print_num.u64_num, data, bytes_to_copy);  
> > > 
> > > This scheme is unlikely to work on big endian machines...  
> > Can you give an example how?
> 
> On BE:
> 
> Input:         [0x01, 0x82]
> Bit length:    15
> Bytes to copy:  2
> bit_offset:     0
> upper_bits:     7
> 
> print_num.u64_num = 0;
> # [0, 0, 0, 0,   0, 0, 0, 0]
> 
> memcpy(&print_num.u64_num, data, bytes_to_copy);  
> # [0x01, 0x82, 0, 0,   0, 0, 0, 0]
> 
> mask = (1 << upper_bits) - 1;
> # mask = 0x7f
> 
> print_num.u8_nums[bytes_to_copy - 1] &= mask;
> # [0x01, 0x02, 0, 0,   0, 0, 0, 0]
> 
> printf("0x%llx", print_num.u64_num);
> # 0x0102000000000000 AKA 72620543991349248
> # expected:
> # 0x0102             AKA 258
> 
> Am I missing something?
yes you're right, good catch! i'll fix this. thanks vrey much :)

> 
> > > > +	upper_bits = BITS_PER_BYTE_MASKED(bits_to_copy);
> > > > +	if (upper_bits) {
> > > > +		uint8_t mask = (1 << upper_bits) - 1;
> > > > +
> > > > +		print_num.u8_nums[bytes_to_copy - 1] &= mask;
> > > > +	}
> > > > +
> > > > +	print_num.u64_num >>= bit_offset;
> > > > +
> > > > +	if (is_plain_text)
> > > > +		jsonw_printf(jw, "0x%llx", print_num.u64_num);
> > > > +	else
> > > > +		jsonw_printf(jw, "%llu", print_num.u64_num);
> > > > +}
> > > > +
> > > > +static int btf_dumper_int(const struct btf_type *t, uint8_t bit_offset,
> > > > +			  const void *data, json_writer_t *jw,
> > > > +			  bool is_plain_text)
> > > > +{
> > > > +	uint32_t *int_type = (uint32_t *)(t + 1);
> > > > +	uint32_t bits = BTF_INT_BITS(*int_type);
> > > > +	int ret = 0;
> > > > +
> > > > +	/* if this is bit field */
> > > > +	if (bit_offset || BTF_INT_OFFSET(*int_type) ||
> > > > +	    BITS_PER_BYTE_MASKED(bits)) {
> > > > +		btf_dumper_int_bits(*int_type, bit_offset, data, jw,
> > > > +				    is_plain_text);
> > > > +		return ret;
> > > > +	}
> > > > +
> > > > +	switch (BTF_INT_ENCODING(*int_type)) {
> > > > +	case 0:
> > > > +		if (BTF_INT_BITS(*int_type) == 64)
> > > > +			jsonw_printf(jw, "%lu", *((uint64_t *)data));
> > > > +		else if (BTF_INT_BITS(*int_type) == 32)
> > > > +			jsonw_printf(jw, "%u", *((uint32_t *)data));
> > > > +		else if (BTF_INT_BITS(*int_type) == 16)
> > > > +			jsonw_printf(jw, "%hu", *((uint16_t *)data));
> > > > +		else if (BTF_INT_BITS(*int_type) == 8)
> > > > +			jsonw_printf(jw, "%hhu", *((uint8_t *)data));
> > > > +		else
> > > > +			btf_dumper_int_bits(*int_type, bit_offset, data, jw,
> > > > +					    is_plain_text);
> > > > +		break;
> > > > +	case BTF_INT_SIGNED:
> > > > +		if (BTF_INT_BITS(*int_type) == 64)
> > > > +			jsonw_printf(jw, "%ld", *((int64_t *)data));
> > > > +		else if (BTF_INT_BITS(*int_type) == 32)
> > > > +			jsonw_printf(jw, "%d", *((int32_t *)data));
> > > > +		else if (BTF_INT_BITS(*int_type) ==  16)  
> > > 
> > > Please drop the double space.  Both for 16 where it makes no sense and
> > > for 8 where it's marginally useful but not really.
> > >   
> > > > +			jsonw_printf(jw, "%hd", *((int16_t *)data));
> > > > +		else if (BTF_INT_BITS(*int_type) ==  8)
> > > > +			jsonw_printf(jw, "%hhd", *((int8_t *)data));
> > > > +		else
> > > > +			btf_dumper_int_bits(*int_type, bit_offset, data, jw,
> > > > +					    is_plain_text);
> > > > +		break;
> > > > +	case BTF_INT_CHAR:
> > > > +		if (*((char *)data) == '\0')
> > > > +			jsonw_null(jw);  
> > > 
> > > Mm.. I don't think 0 char is equivalent to null.  
> > Yes, thanks. Will fix.
> > 
> > >   
> > > > +		else if (isprint(*((char *)data)))
> > > > +			jsonw_printf(jw, "\"%c\"", *((char *)data));  
> > > 
> > > This looks very suspicious.  So if I see a "6" for a char field it's
> > > either a 6 ('\u0006') or a 54 ('6')...  
> > It will always be 54. May be I missed your point. Could you explain why
> > it would be other than 54?
> 
> Ah, I think I missed that %c is in quotes...
> 
> > > > +		else
> > > > +			if (is_plain_text)
> > > > +				jsonw_printf(jw, "%hhx", *((char *)data));
> 
> This seems to be missing a "0x" prefix?
yes it does. will add 0x.

> 
> > > > +			else
> > > > +				jsonw_printf(jw, "%hhd", *((char *)data));  
> > > 
> > > ... I think you need to always print a string, and express it as
> > > \u00%02hhx for non-printable.  
> > Okay that makes sense
> 
> Yeah, IDK, char can be used as a byte as well as a string.  In eBPF
> it may actually be more likely to just be used as a raw byte buffer...
> Either way I think it may be nice to keep it consistent, at least for
> the JSON output could we do either always ints or always characters?
yes, makes sense. i'll keep them always characters.