bpf.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [RFC bpf-next 0/2] Print map ID on successful creation
@ 2025-10-28 12:57 Harshit Mogalapalli
  2025-10-28 12:57 ` [RFC bpf-next 1/2] bpftool: Print map ID upon creation and support JSON output Harshit Mogalapalli
  2025-10-28 12:57 ` [RFC bpf-next 2/2] selftests/bpf: Add test for bpftool map ID printing Harshit Mogalapalli
  0 siblings, 2 replies; 7+ messages in thread
From: Harshit Mogalapalli @ 2025-10-28 12:57 UTC (permalink / raw)
  To: bpf
  Cc: alan.maguire, Harshit Mogalapalli, Quentin Monnet,
	Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko,
	Martin KaFai Lau, Eduard Zingerman, Song Liu, Yonghong Song,
	John Fastabend, KP Singh, Stanislav Fomichev, Hao Luo, Jiri Olsa,
	Shuah Khan, linux-kernel, linux-kselftest

Hi all,

I have tried looking at an issue from the bpftool repository:
https://github.com/libbpf/bpftool/issues/121 and this RFC tries to add
that enhancement.

Summary: Currently when a map creation is successful there is no message
on the terminal, printing IDs on successful creation of maps can help
notify the user and can be used in CI/CD.

The first patch adds the logic for printing and the second patch adds a
simple selftest for the same.

The github issue is not fully solved with these two patches, as there
are other bpf objects that might need similar additions. Would
appreciate any inputs on this.

Thank you very much.

Regards,
Harshit

Harshit Mogalapalli (2):
  bpftool: Print map ID upon creation and support JSON output
  selftests/bpf: Add test for bpftool map ID printing

 tools/bpf/bpftool/map.c                       | 24 +++++++++++---
 .../testing/selftests/bpf/test_bpftool_map.sh | 32 +++++++++++++++++++
 2 files changed, 52 insertions(+), 4 deletions(-)

-- 
2.50.1


^ permalink raw reply	[flat|nested] 7+ messages in thread

* [RFC bpf-next 1/2] bpftool: Print map ID upon creation and support JSON output
  2025-10-28 12:57 [RFC bpf-next 0/2] Print map ID on successful creation Harshit Mogalapalli
@ 2025-10-28 12:57 ` Harshit Mogalapalli
  2025-10-29  2:14   ` Yonghong Song
  2025-10-28 12:57 ` [RFC bpf-next 2/2] selftests/bpf: Add test for bpftool map ID printing Harshit Mogalapalli
  1 sibling, 1 reply; 7+ messages in thread
From: Harshit Mogalapalli @ 2025-10-28 12:57 UTC (permalink / raw)
  To: bpf
  Cc: alan.maguire, Harshit Mogalapalli, Quentin Monnet,
	Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko,
	Martin KaFai Lau, Eduard Zingerman, Song Liu, Yonghong Song,
	John Fastabend, KP Singh, Stanislav Fomichev, Hao Luo, Jiri Olsa,
	Shuah Khan, linux-kernel, linux-kselftest

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);
 		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);
+		goto exit;
+	}
 
+	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);
+	}
 exit:
 	if (attr.inner_map_fd > 0)
 		close(attr.inner_map_fd);
-- 
2.50.1


^ permalink raw reply related	[flat|nested] 7+ messages in thread

* [RFC bpf-next 2/2] selftests/bpf: Add test for bpftool map ID printing
  2025-10-28 12:57 [RFC bpf-next 0/2] Print map ID on successful creation Harshit Mogalapalli
  2025-10-28 12:57 ` [RFC bpf-next 1/2] bpftool: Print map ID upon creation and support JSON output Harshit Mogalapalli
@ 2025-10-28 12:57 ` Harshit Mogalapalli
  1 sibling, 0 replies; 7+ messages in thread
From: Harshit Mogalapalli @ 2025-10-28 12:57 UTC (permalink / raw)
  To: bpf
  Cc: alan.maguire, Harshit Mogalapalli, Quentin Monnet,
	Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko,
	Martin KaFai Lau, Eduard Zingerman, Song Liu, Yonghong Song,
	John Fastabend, KP Singh, Stanislav Fomichev, Hao Luo, Jiri Olsa,
	Shuah Khan, linux-kernel, linux-kselftest

Add selftest to check if Map ID is printed on successful creation in
both plain text and json formats.

Signed-off-by: Harshit Mogalapalli <harshit.m.mogalapalli@oracle.com>
---
 .../testing/selftests/bpf/test_bpftool_map.sh | 36 +++++++++++++++++++
 1 file changed, 36 insertions(+)

diff --git a/tools/testing/selftests/bpf/test_bpftool_map.sh b/tools/testing/selftests/bpf/test_bpftool_map.sh
index 515b1df0501e..013a64e96cbf 100755
--- a/tools/testing/selftests/bpf/test_bpftool_map.sh
+++ b/tools/testing/selftests/bpf/test_bpftool_map.sh
@@ -361,6 +361,40 @@ test_map_access_with_btf_list() {
 	fi
 }
 
+# Function to test map ID printing
+# Parameters:
+#   $1: bpftool path
+#   $2: BPF_DIR
+test_map_id_printing() {
+	local bpftool_path="$1"
+	local bpf_dir="$2"
+	local test_map_name="test_map_id"
+	local test_map_path="$bpf_dir/$test_map_name"
+
+	local output
+	output=$("$bpftool_path" map create "$test_map_path" type hash key 4 \
+		value 8 entries 128 name "$test_map_name")
+	if echo "$output" | grep -q "Map successfully created with ID:"; then
+		echo "PASS: Map ID printed in plain text output."
+	else
+		echo "FAIL: Map ID not printed in plain text output."
+		exit 1
+	fi
+
+	rm -f "$test_map_path"
+
+	output=$("$bpftool_path" map create "$test_map_path" type hash key 4 \
+		value 8 entries 128 name "$test_map_name" --json)
+	if echo "$output" | jq -e 'has("id")' >/dev/null 2>&1; then
+		echo "PASS: Map ID printed in JSON output."
+	else
+		echo "FAIL: Map ID not printed in JSON output."
+		exit 1
+	fi
+
+	rm -f "$test_map_path"
+}
+
 set -eu
 
 trap cleanup_skip EXIT
@@ -395,4 +429,6 @@ test_map_creation_and_map_of_maps "$BPFTOOL_PATH" "$BPF_DIR"
 
 test_map_access_with_btf_list "$BPFTOOL_PATH"
 
+test_map_id_printing "$BPFTOOL_PATH" "$BPF_DIR"
+
 exit 0
-- 
2.50.1


^ permalink raw reply related	[flat|nested] 7+ messages in thread

* Re: [RFC bpf-next 1/2] bpftool: Print map ID upon creation and support JSON output
  2025-10-28 12:57 ` [RFC bpf-next 1/2] bpftool: Print map ID upon creation and support JSON output Harshit Mogalapalli
@ 2025-10-29  2:14   ` Yonghong Song
  2025-10-29  8:48     ` Harshit Mogalapalli
  2025-10-29 16:05     ` Harshit Mogalapalli
  0 siblings, 2 replies; 7+ messages in thread
From: Yonghong Song @ 2025-10-29  2:14 UTC (permalink / raw)
  To: Harshit Mogalapalli, bpf
  Cc: alan.maguire, Quentin Monnet, Alexei Starovoitov, Daniel Borkmann,
	Andrii Nakryiko, Martin KaFai Lau, Eduard Zingerman, Song Liu,
	John Fastabend, KP Singh, Stanislav Fomichev, Hao Luo, Jiri Olsa,
	Shuah Khan, linux-kernel, linux-kselftest



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.

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


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [RFC bpf-next 1/2] bpftool: Print map ID upon creation and support JSON output
  2025-10-29  2:14   ` Yonghong Song
@ 2025-10-29  8:48     ` Harshit Mogalapalli
  2025-10-29 16:05     ` Harshit Mogalapalli
  1 sibling, 0 replies; 7+ messages in thread
From: Harshit Mogalapalli @ 2025-10-29  8:48 UTC (permalink / raw)
  To: Yonghong Song, bpf
  Cc: alan.maguire, Quentin Monnet, Alexei Starovoitov, Daniel Borkmann,
	Andrii Nakryiko, Martin KaFai Lau, Eduard Zingerman, Song Liu,
	John Fastabend, KP Singh, Stanislav Fomichev, Hao Luo, Jiri Olsa,
	Shuah Khan, linux-kernel, linux-kselftest

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.
> 

Thanks a lot for the suggestion.

Will do that and send a v2. Thanks for reviewing.


Regards,
Harshit

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


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [RFC bpf-next 1/2] bpftool: Print map ID upon creation and support JSON output
  2025-10-29  2:14   ` Yonghong Song
  2025-10-29  8:48     ` Harshit Mogalapalli
@ 2025-10-29 16:05     ` Harshit Mogalapalli
  2025-10-29 17:56       ` Yonghong Song
  1 sibling, 1 reply; 7+ messages in thread
From: Harshit Mogalapalli @ 2025-10-29 16:05 UTC (permalink / raw)
  To: Yonghong Song, bpf
  Cc: alan.maguire, Quentin Monnet, Alexei Starovoitov, Daniel Borkmann,
	Andrii Nakryiko, Martin KaFai Lau, Eduard Zingerman, Song Liu,
	John Fastabend, KP Singh, Stanislav Fomichev, Hao Luo, Jiri Olsa,
	Shuah Khan, linux-kernel, linux-kselftest

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);
> 
> 


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [RFC bpf-next 1/2] bpftool: Print map ID upon creation and support JSON output
  2025-10-29 16:05     ` Harshit Mogalapalli
@ 2025-10-29 17:56       ` Yonghong Song
  0 siblings, 0 replies; 7+ messages in thread
From: Yonghong Song @ 2025-10-29 17:56 UTC (permalink / raw)
  To: Harshit Mogalapalli, bpf
  Cc: alan.maguire, Quentin Monnet, Alexei Starovoitov, Daniel Borkmann,
	Andrii Nakryiko, Martin KaFai Lau, Eduard Zingerman, Song Liu,
	John Fastabend, KP Singh, Stanislav Fomichev, Hao Luo, Jiri Olsa,
	Shuah Khan, linux-kernel, linux-kselftest



On 10/29/25 9:05 AM, Harshit Mogalapalli wrote:
> 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);?

The above change LGTM. Thanks!

>
> Thoughts/Suggestions ?
>
> Thanks,
> Harshit
>
>>
>>>       if (attr.inner_map_fd > 0)
>>>           close(attr.inner_map_fd);
>>
>>
>
>


^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, other threads:[~2025-10-29 17:56 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-10-28 12:57 [RFC bpf-next 0/2] Print map ID on successful creation Harshit Mogalapalli
2025-10-28 12:57 ` [RFC bpf-next 1/2] bpftool: Print map ID upon creation and support JSON output Harshit Mogalapalli
2025-10-29  2:14   ` Yonghong Song
2025-10-29  8:48     ` Harshit Mogalapalli
2025-10-29 16:05     ` Harshit Mogalapalli
2025-10-29 17:56       ` Yonghong Song
2025-10-28 12:57 ` [RFC bpf-next 2/2] selftests/bpf: Add test for bpftool map ID printing Harshit Mogalapalli

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).