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.
>
>>
>>
>>> + 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.
[...]
