netdev - Re: [PATCH net-next v2 09/11] net/mlx5e: Add TX reporter support

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20190121134532.GG2228@nanopsycho>
Date:   Mon, 21 Jan 2019 14:45:32 +0100
From:   Jiri Pirko <jiri@...nulli.us>
To:     Eran Ben Elisha <eranbe@...lanox.com>
Cc:     "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        Jiri Pirko <jiri@...lanox.com>,
        "David S. Miller" <davem@...emloft.net>,
        Ariel Almog <ariela@...lanox.com>,
        Aya Levin <ayal@...lanox.com>,
        Moshe Shemesh <moshe@...lanox.com>
Subject: Re: [PATCH net-next v2 09/11] net/mlx5e: Add TX reporter support

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