Re: [PATCH v5 2/5] telemetry: fix repeated display when callback don't set dict

[fengchengwen <fengchengwen@huawei.com>]

Hi Bruce,

On 2023/1/7 1:33, Bruce Richardson wrote:
> On Fri, Jan 06, 2023 at 04:07:45PM +0000, Bruce Richardson wrote:
>> On Mon, Dec 26, 2022 at 12:53:57PM +0800, fengchengwen wrote:
>>> On 2022/12/19 17:33, Bruce Richardson wrote:
>>>> On Mon, Dec 19, 2022 at 09:07:20AM +0000, Chengwen Feng wrote:
>>>>> When telemetry callback didn't set dict and return a non-negative
>>>>> number, the telemetry will repeat to display the last result.
>>>>>
>>>>> Fixes: 6dd571fd07c3 ("telemetry: introduce new functionality")
>>>>> Cc: stable@dpdk.org
>>>>>
>>>>> Signed-off-by: Chengwen Feng <fengchengwen@huawei.com>
>>>>> ---
>>>>
>>>> Hi Chengwen,
>>>>
>>>> I'm a little curious about this bug. Can you describe some steps to
>>>> reproduce it as I'm curious as to exactly what is happening. The fix seems
>>>> a little strange to me so I'd like to investigate a little more to see if
>>>> other approaches might work.
>>>
>>> Hi Bruce,
>>>
>>> Sorry for late reply.
>>>
>>> The steps:
>>>   1. applay "[PATCH v5 1/5] dmadev: support stats reset telemetry command"
>>>   2. compile
>>>   3. start dpdk-dma: dpdk-dma -a DMA.BDF -a NIC.BDF -- -c hw
>>>   4. start telemetry, and execute /dmadev/stats,0, and then /dmadev/stats_reset,0
>>>      the output of /dmadev/stats_reset,0 will be the same of previous cmd "/dmadev/stats,0"
>>>      e.g. my environment:
>>> --> /dmadev/stats,0
>>> {
>>>   "/dmadev/stats": {
>>>     "submitted": 23,
>>>     "completed": 23,
>>>     "errors": 0
>>>   }
>>> }
>>> --> /dmadev/stats_reset,0
>>> {
>>>   "/dmadev/stats_reset": {
>>>     "submitted": 23,
>>>     "completed": 23,
>>>     "errors": 0
>>>   }
>>> }
>>>
>>> The rootcause is that the /dmadev/stats_reset don't set the outer parameter "struct rte_tel_data *info"
>>> and return zero.
>>>
>> Thanks for the fuller explanation, I'll hopefully test it out myself.
>>
>> However, in the meantime, looking at the telemetry library code, would the
>> following change work rather than explicitly always setting the telemetry
>> data to a dictionary by default? Zeroing the data by default sets it to a
>> null return which is what you probably want as default rather than an empty
>> dictionary. (And it's also a smaller diff)
>>
>> /Bruce
>>
>> diff --git a/lib/telemetry/telemetry.c b/lib/telemetry/telemetry.c
>> index 8fbb4f3060..7b905355cd 100644
>> --- a/lib/telemetry/telemetry.c
>> +++ b/lib/telemetry/telemetry.c
>> @@ -333,7 +333,7 @@ output_json(const char *cmd, const struct rte_tel_data *d, int s)
>>  static void
>>  perform_command(telemetry_cb fn, const char *cmd, const char *param, int s)
>>  {
>> -       struct rte_tel_data data;
>> +       struct rte_tel_data data = {0};
>>  
>>         int ret = fn(cmd, param, &data);
>>         if (ret < 0) {
>>
> 
> I've handily reproduced the issue using the instructions you gave above,
> thanks for those.
> 
> Based on that, and looking a little deeper:
> 
> * I think it is an error with the reset function not to initialize and
>   complete the return value. I believe that for each telemetry callback we
>   should require that the callback fill in a valid value on success. For
>   reset, some options could be just a string "OK", or an array just
>   containing "[0]", or similar.

good idea, the v2 follow it.

> 
> * Given that point above, I do agree though that the "data" parameter
>   should be properly initialized on entry to the callbacks. However, I feel
>   that the correct init should be the empty/null value, as is given in the
>   patch above.

already fix in v2

Thanks.

> 
> Regards,
> /Bruce
> .
>