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".
>>>
>>> The following is the output when using bpftool to manipulate
>>> qp-trie:
>>>
>>> $ bpftool map pin id 724953 /sys/fs/bpf/qp
>>> $ bpftool map show pinned /sys/fs/bpf/qp
>>> 724953: qp_trie name qp_trie flags 0x1
>>> key 16B value 4B max_entries 2 memlock 65536B map_extra 8
>>> btf_id 779
>>> pids test_qp_trie.bi(109167)
>>> $ bpftool map dump pinned /sys/fs/bpf/qp
>>> [{
>>> "key": {
>>> "size": 4,
>>> "data": ["0x0","0x0","0x0","0x0"
>>> ]
>>> },
>>> "value": 0
>>> },{
>>> "key": {
>>> "size": 4,
>>> "data": ["0x0","0x0","0x0","0x1"
>>> ]
>>> },
>>> "value": 2
>>> }
>>> ]
>>> $ bpftool map lookup pinned /sys/fs/bpf/qp key 4 0 0 0 1
>>> {
>>> "key": {
>>> "size": 4,
>>> "data": ["0x0","0x0","0x0","0x1"
>>> ]
>>> },
>>> "value": 2
>>> }
>> 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?
> 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.
