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: sta...@dpdk.org
> > >>
> > >> Signed-off-by: Chengwen Feng <fengcheng...@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.
* 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.
Regards,
/Bruce
