Mon, Jan 21, 2019 at 02:06:44PM CET, era...@mellanox.com wrote:
>
>
>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.
If you have 8 SQs, you should have 8 objects (with subobjects) in an
array.
>
>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.
>
>
>>
>> [...]
>>
