On 1/21/2019 2:11 PM, Jiri Pirko wrote:

Mon, Jan 21, 2019 at 12:32:07PM CET, eranbe@mellanox.com wrote:

quoted

On 1/20/2019 1:06 PM, Jiri Pirko wrote:

quoted

Thu, Jan 17, 2019 at 10:59:18PM CET, eranbe@mellanox.com wrote:

[...]

	

quoted

quoted

quoted

+static int
+mlx5e_tx_reporter_build_diagnose_output(struct devlink_health_buffer *buffer,
+					u32 sqn, u8 state, u8 stopped)
+{
+	int err, i;
+	int nest = 0;
+	char name[20];
+
+	err = devlink_health_buffer_nest_start(buffer,
+					       DEVLINK_ATTR_HEALTH_BUFFER_OBJECT);
+	if (err)
+		goto buffer_error;
+	nest++;
+
+	err = devlink_health_buffer_nest_start(buffer,
+					       DEVLINK_ATTR_HEALTH_BUFFER_OBJECT_PAIR);
+	if (err)
+		goto buffer_error;
+	nest++;
+
+	sprintf(name, "SQ 0x%x", sqn);

No. The whole idea of having the message build up with nested attributes
(json-like) was to avoid things like this. No sprintfs please. If you
want to do sprintf, most of the time you are doing something wrong.

I wanted that each SQ object will have a unique name. But I can merge
the sqn into its attributes instead.

Should be an array.

Every SQ shall hold it's own properties. I don't see why all SQs shall 
be held under one array, this array do not provide any additional 
info/order.

In addition, it is better to keep main preliminary objects up to the 
size of one netlink SKB, otherwise it will be impossible for the devlink 
to prepare this one big object as skb fragments, and also to re-assmble 
them in user space as driver intended them to be.

If all SWs will be under one big array, under one big object, we will 
hit this corner case.

quoted

quoted

quoted

+	err = devlink_health_buffer_put_object_name(buffer, name);
+	if (err)
+		goto buffer_error;
+
+	err = devlink_health_buffer_nest_start(buffer,
+					       DEVLINK_ATTR_HEALTH_BUFFER_OBJECT_VALUE);
+	if (err)
+		goto buffer_error;
+	nest++;
+
+	err = devlink_health_buffer_nest_start(buffer,
+					       DEVLINK_ATTR_HEALTH_BUFFER_OBJECT);
+	if (err)
+		goto buffer_error;
+	nest++;
+
+	err = devlink_health_buffer_nest_start(buffer,
+					       DEVLINK_ATTR_HEALTH_BUFFER_OBJECT_PAIR);
+	if (err)
+		goto buffer_error;
+	nest++;
+
+	err = devlink_health_buffer_put_object_name(buffer, "HW state");
+	if (err)
+		goto buffer_error;
+
+	err = devlink_health_buffer_nest_start(buffer,
+					       DEVLINK_ATTR_HEALTH_BUFFER_OBJECT_VALUE);

How could you put an object name and don't start nesting? It looks
implicit to me.

I will add some helper functions that you could review. Just keep in
mind the implicit nest start must come with implicit nest end, so it
won't fit into every use...

You can do just object_start(), object_finish() or something like that.

ack.

Better to have also helpers that start and close their nests on one 
shot. like pair_name_value or pair_name_value_array.

Also, The json is too powerful to close it inside few wrappers, we must 
give the basic blocks as well for nontraditional structures.

[...]