diff mbox series

[iproute2-next,v3,3/6] devlink: extend pr_out_nested_handle() to print object

Message ID 20231024100403.762862-4-jiri@resnulli.us (mailing list archive)
State Superseded
Delegated to: David Ahern
Headers show
Series expose devlink instances relationships | expand

Checks

Context Check Description
netdev/tree_selection success Not a local patch

Commit Message

Jiri Pirko Oct. 24, 2023, 10:04 a.m. UTC 
From: Jiri Pirko <jiri@nvidia.com>

For existing pr_out_nested_handle() user (line card), the output stays
the same. For the new users, introduce __pr_out_nested_handle()
to allow to print devlink instance as object allowing to carry
attributes in it (like netns).

Note that as __pr_out_handle_start() and pr_out_handle_end() are newly
used, the function is moved below the definitions.

Signed-off-by: Jiri Pirko <jiri@nvidia.com>
---
v2->v3:
- new patch
---
 devlink/devlink.c | 51 +++++++++++++++++++++++++++++------------------
 1 file changed, 32 insertions(+), 19 deletions(-)

Comments

David Ahern Oct. 26, 2023, 5:03 p.m. UTC | #1 
On 10/24/23 4:04 AM, Jiri Pirko wrote:
> @@ -2861,6 +2842,38 @@ static void pr_out_selftests_handle_end(struct dl *dl)
>  		__pr_out_newline();
>  }
>  
> +static void __pr_out_nested_handle(struct dl *dl, struct nlattr *nla_nested_dl,
> +				   bool is_object)
> +{
> +	struct nlattr *tb[DEVLINK_ATTR_MAX + 1] = {};
> +	int err;
> +
> +	err = mnl_attr_parse_nested(nla_nested_dl, attr_cb, tb);
> +	if (err != MNL_CB_OK)
> +		return;
> +
> +	if (!tb[DEVLINK_ATTR_BUS_NAME] ||
> +	    !tb[DEVLINK_ATTR_DEV_NAME])
> +		return;
> +
> +	if (!is_object) {
> +		char buf[64];
> +
> +		sprintf(buf, "%s/%s", mnl_attr_get_str(tb[DEVLINK_ATTR_BUS_NAME]),
> +			mnl_attr_get_str(tb[DEVLINK_ATTR_DEV_NAME]));

buf[64] - 1 for null terminator - 16 for IFNAMSIZ leaves 47. I do not
see limits on bus name length, so how can you guarantee it is always <
47 characters?

Make this snprintf, check the return and make sure buf is null terminated.

> +		print_string(PRINT_ANY, "nested_devlink", " nested_devlink %s", buf);
> +		return;
> +	}
> +
> +	__pr_out_handle_start(dl, tb, false, false);
> +	pr_out_handle_end(dl);
> +}
> +
> +static void pr_out_nested_handle(struct nlattr *nla_nested_dl)
> +{
> +	__pr_out_nested_handle(NULL, nla_nested_dl, false);
> +}
> +
>  static bool cmp_arr_last_port_handle(struct dl *dl, const char *bus_name,
>  				     const char *dev_name, uint32_t port_index)
>  {
Jiri Pirko Oct. 27, 2023, 8:26 a.m. UTC | #2 
Thu, Oct 26, 2023 at 07:03:30PM CEST, dsahern@gmail.com wrote:
>On 10/24/23 4:04 AM, Jiri Pirko wrote:
>> @@ -2861,6 +2842,38 @@ static void pr_out_selftests_handle_end(struct dl *dl)
>>  		__pr_out_newline();
>>  }
>>  
>> +static void __pr_out_nested_handle(struct dl *dl, struct nlattr *nla_nested_dl,
>> +				   bool is_object)
>> +{
>> +	struct nlattr *tb[DEVLINK_ATTR_MAX + 1] = {};
>> +	int err;
>> +
>> +	err = mnl_attr_parse_nested(nla_nested_dl, attr_cb, tb);
>> +	if (err != MNL_CB_OK)
>> +		return;
>> +
>> +	if (!tb[DEVLINK_ATTR_BUS_NAME] ||
>> +	    !tb[DEVLINK_ATTR_DEV_NAME])
>> +		return;
>> +
>> +	if (!is_object) {
>> +		char buf[64];
>> +
>> +		sprintf(buf, "%s/%s", mnl_attr_get_str(tb[DEVLINK_ATTR_BUS_NAME]),
>> +			mnl_attr_get_str(tb[DEVLINK_ATTR_DEV_NAME]));
>
>buf[64] - 1 for null terminator - 16 for IFNAMSIZ leaves 47. I do not

IFNAMSIZ is irrelevant here.


>see limits on bus name length, so how can you guarantee it is always <
>47 characters?
>
>Make this snprintf, check the return and make sure buf is null terminated.

I will fix it in separate patch, as this is just copied here.


>
>> +		print_string(PRINT_ANY, "nested_devlink", " nested_devlink %s", buf);
>> +		return;
>> +	}
>> +
>> +	__pr_out_handle_start(dl, tb, false, false);
>> +	pr_out_handle_end(dl);
>> +}
>> +
>> +static void pr_out_nested_handle(struct nlattr *nla_nested_dl)
>> +{
>> +	__pr_out_nested_handle(NULL, nla_nested_dl, false);
>> +}
>> +
>>  static bool cmp_arr_last_port_handle(struct dl *dl, const char *bus_name,
>>  				     const char *dev_name, uint32_t port_index)
>>  {
>
Petr Machata Oct. 27, 2023, 1:12 p.m. UTC | #3 
David Ahern <dsahern@gmail.com> writes:

> On 10/24/23 4:04 AM, Jiri Pirko wrote:
>> @@ -2861,6 +2842,38 @@ static void pr_out_selftests_handle_end(struct dl *dl)
>>  		__pr_out_newline();
>>  }
>>  
>> +static void __pr_out_nested_handle(struct dl *dl, struct nlattr *nla_nested_dl,
>> +				   bool is_object)
>> +{
>> +	struct nlattr *tb[DEVLINK_ATTR_MAX + 1] = {};
>> +	int err;
>> +
>> +	err = mnl_attr_parse_nested(nla_nested_dl, attr_cb, tb);
>> +	if (err != MNL_CB_OK)
>> +		return;
>> +
>> +	if (!tb[DEVLINK_ATTR_BUS_NAME] ||
>> +	    !tb[DEVLINK_ATTR_DEV_NAME])
>> +		return;
>> +
>> +	if (!is_object) {
>> +		char buf[64];
>> +
>> +		sprintf(buf, "%s/%s", mnl_attr_get_str(tb[DEVLINK_ATTR_BUS_NAME]),
>> +			mnl_attr_get_str(tb[DEVLINK_ATTR_DEV_NAME]));
>
> buf[64] - 1 for null terminator - 16 for IFNAMSIZ leaves 47. I do not
> see limits on bus name length, so how can you guarantee it is always <
> 47 characters?
>
> Make this snprintf, check the return and make sure buf is null terminated.

I was wondering whether somehing like this might make sense in the
iproute2 library:

	#define alloca_sprintf(FMT, ...) ({					\
		int xasprintf_n = snprintf(NULL, 0, (FMT), __VA_ARGS__);	\
		char *xasprintf_buf = alloca(xasprintf_n);			\
		sprintf(xasprintf_buf, (FMT), __VA_ARGS__);			\
		xasprintf_buf;							\
	})

	void foo() {
		const char *buf = alloca_sprintf("%x %y %z", etc.);
		printf(... buf ...);
	}

I'm not really happy with it -- because of alloca vs. array, and because
of the double evaluation. But all those SPRINT_BUF's peppered everywhere
make me uneasy every time I read or write them.

Or maybe roll something custom asprintf-like that can reuse and/or
realloc a passed-in buffer?

The sprintf story is pretty bad in iproute2 right now, IMHO.
David Ahern Oct. 27, 2023, 5:16 p.m. UTC | #4 
On 10/27/23 7:12 AM, Petr Machata wrote:
> I was wondering whether somehing like this might make sense in the
> iproute2 library:
> 
> 	#define alloca_sprintf(FMT, ...) ({					\
> 		int xasprintf_n = snprintf(NULL, 0, (FMT), __VA_ARGS__);	\
> 		char *xasprintf_buf = alloca(xasprintf_n);			\
> 		sprintf(xasprintf_buf, (FMT), __VA_ARGS__);			\
> 		xasprintf_buf;							\
> 	})
> 
> 	void foo() {
> 		const char *buf = alloca_sprintf("%x %y %z", etc.);
> 		printf(... buf ...);
> 	}
> 
> I'm not really happy with it -- because of alloca vs. array, and because
> of the double evaluation. But all those SPRINT_BUF's peppered everywhere
> make me uneasy every time I read or write them.

agreed.

> 
> Or maybe roll something custom asprintf-like that can reuse and/or
> realloc a passed-in buffer?
> 
> The sprintf story is pretty bad in iproute2 right now, IMHO.

It is a bit of a mess. If you have a few cycles, want to send an RFC?
Just pick 1 or 2 to convert to show intent with a new design.
Petr Machata Oct. 30, 2023, 11:03 a.m. UTC | #5 
David Ahern <dsahern@gmail.com> writes:

> On 10/27/23 7:12 AM, Petr Machata wrote:
>> I was wondering whether somehing like this might make sense in the
>> iproute2 library:
>> 
>> 	#define alloca_sprintf(FMT, ...) ({					\
>> 		int xasprintf_n = snprintf(NULL, 0, (FMT), __VA_ARGS__);	\
>> 		char *xasprintf_buf = alloca(xasprintf_n);			\
>> 		sprintf(xasprintf_buf, (FMT), __VA_ARGS__);			\
>> 		xasprintf_buf;							\
>> 	})
>> 
>> 	void foo() {
>> 		const char *buf = alloca_sprintf("%x %y %z", etc.);
>> 		printf(... buf ...);
>> 	}
>> 
>> I'm not really happy with it -- because of alloca vs. array, and because
>> of the double evaluation. But all those SPRINT_BUF's peppered everywhere
>> make me uneasy every time I read or write them.
>
> agreed.
>
>> 
>> Or maybe roll something custom asprintf-like that can reuse and/or
>> realloc a passed-in buffer?
>> 
>> The sprintf story is pretty bad in iproute2 right now, IMHO.
>
> It is a bit of a mess. If you have a few cycles, want to send an RFC?
> Just pick 1 or 2 to convert to show intent with a new design.

I picked at it a bit over the weekend, but came up with nothing that I
find comfortable proposing. The static buffer approach has some major
advantages: nothing ever fails and nothing ever needs cleanups. This
keeps the client code tidy and compact. Anything dynamic adds points of
failure and cleanups, which in C means more client-side boilerplate.
Anyway, I'll pick at it some more and see I find anything.
diff mbox series

Patch

diff --git a/devlink/devlink.c b/devlink/devlink.c
index c18a4a4fbc5a..f7325477f271 100644
--- a/devlink/devlink.c
+++ b/devlink/devlink.c
@@ -2747,25 +2747,6 @@  static bool should_arr_last_handle_end(struct dl *dl, const char *bus_name,
 	       !cmp_arr_last_handle(dl, bus_name, dev_name);
 }
 
-static void pr_out_nested_handle(struct nlattr *nla_nested_dl)
-{
-	struct nlattr *tb[DEVLINK_ATTR_MAX + 1] = {};
-	char buf[64];
-	int err;
-
-	err = mnl_attr_parse_nested(nla_nested_dl, attr_cb, tb);
-	if (err != MNL_CB_OK)
-		return;
-
-	if (!tb[DEVLINK_ATTR_BUS_NAME] ||
-	    !tb[DEVLINK_ATTR_DEV_NAME])
-		return;
-
-	sprintf(buf, "%s/%s", mnl_attr_get_str(tb[DEVLINK_ATTR_BUS_NAME]),
-		mnl_attr_get_str(tb[DEVLINK_ATTR_DEV_NAME]));
-	print_string(PRINT_ANY, "nested_devlink", " nested_devlink %s", buf);
-}
-
 static void __pr_out_handle_start(struct dl *dl, struct nlattr **tb,
 				  bool content, bool array)
 {
@@ -2861,6 +2842,38 @@  static void pr_out_selftests_handle_end(struct dl *dl)
 		__pr_out_newline();
 }
 
+static void __pr_out_nested_handle(struct dl *dl, struct nlattr *nla_nested_dl,
+				   bool is_object)
+{
+	struct nlattr *tb[DEVLINK_ATTR_MAX + 1] = {};
+	int err;
+
+	err = mnl_attr_parse_nested(nla_nested_dl, attr_cb, tb);
+	if (err != MNL_CB_OK)
+		return;
+
+	if (!tb[DEVLINK_ATTR_BUS_NAME] ||
+	    !tb[DEVLINK_ATTR_DEV_NAME])
+		return;
+
+	if (!is_object) {
+		char buf[64];
+
+		sprintf(buf, "%s/%s", mnl_attr_get_str(tb[DEVLINK_ATTR_BUS_NAME]),
+			mnl_attr_get_str(tb[DEVLINK_ATTR_DEV_NAME]));
+		print_string(PRINT_ANY, "nested_devlink", " nested_devlink %s", buf);
+		return;
+	}
+
+	__pr_out_handle_start(dl, tb, false, false);
+	pr_out_handle_end(dl);
+}
+
+static void pr_out_nested_handle(struct nlattr *nla_nested_dl)
+{
+	__pr_out_nested_handle(NULL, nla_nested_dl, false);
+}
+
 static bool cmp_arr_last_port_handle(struct dl *dl, const char *bus_name,
 				     const char *dev_name, uint32_t port_index)
 {