Re: [RFC bpf-next 1/2] bpftool: Print map ID upon creation and support JSON output

[Harshit Mogalapalli <harshit.m.mogalapalli@oracle.com>]

Hi Yonghong,


On 29/10/25 07:44, Yonghong Song wrote:
> 
> 
> On 10/28/25 5:57 AM, Harshit Mogalapalli wrote:
>> It is useful to print map ID on successful creation.
>>
>> JSON case:
>> $ ./bpftool -j map create /sys/fs/bpf/test_map4 type hash key 4 value 
>> 8 entries 128 name map4
>> {"id":12}
>>
>> Generic case:
>> $ ./bpftool  map create /sys/fs/bpf/test_map5 type hash key 4 value 8 
>> entries 128 name map5
>> Map successfully created with ID: 15
>>
>> Bpftool Issue: https://github.com/libbpf/bpftool/issues/121
>> Signed-off-by: Harshit Mogalapalli <harshit.m.mogalapalli@oracle.com>
>> ---
>>   tools/bpf/bpftool/map.c | 24 ++++++++++++++++++++----
>>   1 file changed, 20 insertions(+), 4 deletions(-)
>>
>> diff --git a/tools/bpf/bpftool/map.c b/tools/bpf/bpftool/map.c
>> index c9de44a45778..b6580f25361d 100644
>> --- a/tools/bpf/bpftool/map.c
>> +++ b/tools/bpf/bpftool/map.c
>> @@ -1251,6 +1251,8 @@ static int do_create(int argc, char **argv)
>>       LIBBPF_OPTS(bpf_map_create_opts, attr);
>>       enum bpf_map_type map_type = BPF_MAP_TYPE_UNSPEC;
>>       __u32 key_size = 0, value_size = 0, max_entries = 0;
>> +    struct bpf_map_info map_info = {};
>> +    __u32 map_info_len = sizeof(map_info);
>>       const char *map_name = NULL;
>>       const char *pinfile;
>>       int err = -1, fd;
>> @@ -1353,13 +1355,27 @@ static int do_create(int argc, char **argv)
>>       }
>>       err = do_pin_fd(fd, pinfile);
>> -    close(fd);
>> -    if (err)
>> +    if (err) {
>> +        close(fd);
> 
> I think you can remove close(fd) here,
> 
>>           goto exit;
>> +    }
>> -    if (json_output)
>> -        jsonw_null(json_wtr);
>> +    err = bpf_obj_get_info_by_fd(fd, &map_info, &map_info_len);
>> +    if (err) {
>> +        p_err("Failed to fetch map info: %s\n", strerror(errno));
>> +        close(fd);
> 
> and here
> 
>> +        goto exit;
>> +    }
>> +    close(fd);
> 
> and here,
> 
>> +
>> +    if (json_output) {
>> +        jsonw_start_object(json_wtr);
>> +        jsonw_int_field(json_wtr, "id", map_info.id);
>> +        jsonw_end_object(json_wtr);
>> +    } else {
>> +        printf("Map successfully created with ID: %u\n", map_info.id);
>> +    }
>>   exit:
> 
> and put close(fd) here.

I think we need one more close_fd: label and then put a close(fd); here. 
As there are other gotos to exit earlier in this function when fd is 
uninitialized, which can the error like:

map.c: In function ‘do_create’:
map.c:1375:9: warning: ‘fd’ may be used uninitialized 
[-Wmaybe-uninitialized]
  1375 |         close(fd);
       |         ^~~~~~~~~
map.c:1258:23: note: ‘fd’ was declared here
  1258 |         int err = -1, fd;
       |                       ^~



So, maybe we could do something like this:

         err = do_pin_fd(fd, pinfile);
-       close(fd);
         if (err)
-               goto exit;
+               goto close_fd;

-       if (json_output)
-               jsonw_null(json_wtr);
+       err = bpf_obj_get_info_by_fd(fd, &map_info, &map_info_len);
+       if (err) {
+               p_err("Failed to fetch map info: %s\n", strerror(errno));
+               goto close_fd;
+       }

+       if (json_output) {
+               jsonw_start_object(json_wtr);
+               jsonw_int_field(json_wtr, "id", map_info.id);
+               jsonw_end_object(json_wtr);
+       } else {
+               printf("Map successfully created with ID: %u\n", 
map_info.id);
+       }
+close_fd:
+       close(fd);
  exit:
         if (attr.inner_map_fd > 0)
                 close(attr.inner_map_fd);

I can prepare a v2 with this change, but wouldn't it be simpler to add a
direct close(fd); on the few error paths instead of introducing an
additional label for close(fd);?

Thoughts/Suggestions ?

Thanks,
Harshit

> 
>>       if (attr.inner_map_fd > 0)
>>           close(attr.inner_map_fd);
> 
>