Hi,
On 9/28/2022 4:40 PM, Quentin Monnet wrote:
> Wed Sep 28 2022 05:14:45 GMT+0100 (British Summer Time) ~ Hou Tao
> <houtao@xxxxxxxxxxxxxxx>
>> Hi,
>>
>> On 9/27/2022 7:24 PM, Quentin Monnet wrote:
>>> Sat Sep 24 2022 14:36:15 GMT+0100 (British Summer Time) ~ Hou Tao
>>> <houtao@xxxxxxxxxxxxxxx>
>>>> From: Hou Tao <houtao1@xxxxxxxxxx>
>>>>
>>>> Support lookup/update/delete/iterate/dump operations for qp-trie in
>>>> bpftool. Mainly add two functions: one function to parse dynptr key and
>>>> another one to dump dynptr key. The input format of dynptr key is:
>>>> "key [hex] size BYTES" and the output format of dynptr key is:
>>>> "size BYTES".
SNIP
>>> The bpftool patch looks good, thanks! I have one comment on the syntax
>>> for the keys, I don't find it intuitive to have the size as the first
>>> BYTE. It makes it awkward to understand what the command does if we read
>>> it in the wild without knowing the map type. I can see two alternatives,
>>> either adding a keyword (e.g., "key_size 4 key 0 0 0 1"), or changing
>>> parse_bytes() to make it able to parse as much as it can then count the
>>> bytes, when we don't know in advance how many we get.
>> The suggestion is reasonable, but there is also reason for the current choice (
>> I should written it down in commit message). For dynptr-typed key, these two
>> proposed suggestions will work. But for key with embedded dynptrs as show below,
>> both explict key_size keyword and implicit key_size in BYTEs can not express the
>> key correctly.
>>
>> struct map_key {
>> unsigned int cookie;
>> struct bpf_dynptr name;
>> struct bpf_dynptr addr;
>> unsigned int flags;
>> };
> I'm not sure I follow. I don't understand the difference for dealing
> internally with the key between "key_size N key BYTES" and "key N BYTES"
> (or for parsing then counting). Please could you give an example telling
> how you would you express the key from the structure above, with the
> syntax you proposed?
In my understand, if using "key_size N key BYTES" to represent map_key, it can
not tell the exact size of "name" and "addr" and it only can tell the total size
of name and addr. If using "key BYTES" to do that, it has the similar problem.
But if using "key size BYTES" format, map_key can be expressed as follows:
key c c c c [name_size] n n n [addr_size] a a f f f f
>
>> I also had thought about adding another key word "dynptr_key" (or "dyn_key") to
>> support dynptr-typed key or key with embedded dynptr, and the format will still
>> be: "dynptr_key size [BYTES]". But at least we can tell it is different with
>> "key" which is fixed size. What do you think ?
> If the other suggestions do not work, then yes, using a dedicated
> keyword (Just "dynkey"? We can detail in the docs) sounds better to me.
