On 1/21/2019 2:11 PM, Jiri Pirko wrote:
> Mon, Jan 21, 2019 at 12:32:07PM CET, era...@mellanox.com wrote:
>>
>>
>> On 1/20/2019 1:06 PM, Jiri Pirko wrote:
>>> Thu, Jan 17, 2019 at 10:59:18PM CET, era...@mellanox.com wrote:
>
> [...]
>
>
>>>> +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.
>
>
>>
>>>
>>>
>>>> + 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.
>
> [...]
>
