DPDK patches and discussions
 help / color / mirror / Atom feed
From: David Marchand <david.marchand@redhat.com>
To: Bruce Richardson <bruce.richardson@intel.com>
Cc: dev@dpdk.org, mb@smartsharesystems.com,
	 Ciara Power <ciara.power@intel.com>
Subject: Re: [PATCH] telemetry: support boolean type
Date: Wed, 19 Oct 2022 16:28:58 +0200	[thread overview]
Message-ID: <CAJFAV8z+en7A1n4RLpooHWLFe4wpJVyk2Q-0_SHOm-U_MgEKOg@mail.gmail.com> (raw)
In-Reply-To: <Y1AABobg98qzGTgE@bricha3-MOBL.ger.corp.intel.com>

On Wed, Oct 19, 2022 at 3:48 PM Bruce Richardson
<bruce.richardson@intel.com> wrote:
>
> On Wed, Oct 19, 2022 at 09:37:02AM +0200, David Marchand wrote:
> > Add the boolean type RTE_TEL_BOOL_VAL for values in arrays and dicts.
> >
> > Signed-off-by: David Marchand <david.marchand@redhat.com>
> > ---
>
> This patch looks pretty good to me. Some very small comments inline below.
> One thing I notice is that we are not supporting booleans except as part of
> an array or dictionary. Is it likely that we will ever want to have a
> telemetry command that just returns true/false alone? Don't see that being

I wondered too, but then I saw that only the "simple" type string was
handled and others were not.
So I decided to skip.


> necessary just yet, so:
>
> Reviewed-by: Bruce Richardson <bruce.richardson@intel.com>
> Acked-by: Bruce Richardson <bruce.richardson@intel.com>
>
> >  app/test/test_telemetry_data.c | 88 +++++++++++++++++++++++++++++++++-
> >  lib/telemetry/rte_telemetry.h  | 36 ++++++++++++++
> >  lib/telemetry/telemetry.c      | 24 +++++++++-
> >  lib/telemetry/telemetry_data.c | 44 +++++++++++++++--
> >  lib/telemetry/telemetry_data.h |  5 ++
> >  lib/telemetry/telemetry_json.h | 34 +++++++++++++
> >  lib/telemetry/version.map      |  5 ++
> >  7 files changed, 228 insertions(+), 8 deletions(-)
> >
>
> <snip>
>
> > diff --git a/lib/telemetry/rte_telemetry.h b/lib/telemetry/rte_telemetry.h
> > index a0d21d6b7f..5d74212f17 100644
> > --- a/lib/telemetry/rte_telemetry.h
> > +++ b/lib/telemetry/rte_telemetry.h
> > @@ -2,6 +2,7 @@
> >   * Copyright(c) 2018 Intel Corporation
> >   */
> >
> > +#include <stdbool.h>
> >  #include <stdint.h>
> >
> >  #include <rte_compat.h>
> > @@ -46,6 +47,7 @@ enum rte_tel_value_type {
> >       RTE_TEL_INT_VAL,    /** a signed 32-bit int value */
> >       RTE_TEL_U64_VAL,    /** an unsigned 64-bit int value */
> >       RTE_TEL_CONTAINER, /** a container struct */
> > +     RTE_TEL_BOOL_VAL,   /** a boolean value */
> >  };
> >
> >  /**
> > @@ -155,6 +157,22 @@ int
> >  rte_tel_data_add_array_container(struct rte_tel_data *d,
> >               struct rte_tel_data *val, int keep);
> >
> > +/**
> > + * Add a boolean to an array.
> > + * The array must have been started by rte_tel_data_start_array() with
> > + * RTE_TEL_BOOL_VAL as the type parameter.
> > + *
> > + * @param d
> > + *   The data structure passed to the callback
> > + * @param x
> > + *   The number to be returned in the array
>
> number -> boolean value
>

Indeed..

> > + * @return
> > + *   0 on success, negative errno on error
> > + */
> > +__rte_experimental
> > +int
> > +rte_tel_data_add_array_bool(struct rte_tel_data *d, bool x);
> > +
> >  /**
> >   * Add a string value to a dictionary.
> >   * The dict must have been started by rte_tel_data_start_dict().
> > @@ -233,6 +251,24 @@ int
> >  rte_tel_data_add_dict_container(struct rte_tel_data *d, const char *name,
> >               struct rte_tel_data *val, int keep);
> >
> > +/**
> > + * Add a boolean value to a dictionary.
> > + * The dict must have been started by rte_tel_data_start_dict().
> > + *
> > + * @param d
> > + *   The data structure passed to the callback
> > + * @param name
> > + *   The name the value is to be stored under in the dict
> > + *   Must contain only alphanumeric characters or the symbols: '_' or '/'
> > + * @param val
> > + *   The number to be stored in the dict
>
> number -> boolean value
>
> > + * @return
> > + *   0 on success, negative errno on error, E2BIG on string truncation of name.
> > + */
> > +__rte_experimental
> > +int
> > +rte_tel_data_add_dict_bool(struct rte_tel_data *d, const char *name, bool val);
> > +
> >  /**
> >   * This telemetry callback is used when registering a telemetry command.
> >   * It handles getting and formatting information to be returned to telemetry
> > diff --git a/lib/telemetry/telemetry.c b/lib/telemetry/telemetry.c
> <snip>
> > diff --git a/lib/telemetry/telemetry_data.c b/lib/telemetry/telemetry_data.c
> > index 5b319c18fb..4f81f71e03 100644
> > --- a/lib/telemetry/telemetry_data.c
> > +++ b/lib/telemetry/telemetry_data.c
> > @@ -16,10 +16,11 @@ int
> >  rte_tel_data_start_array(struct rte_tel_data *d, enum rte_tel_value_type type)
> >  {
> >       enum tel_container_types array_types[] = {
> > -                     RTE_TEL_ARRAY_STRING, /* RTE_TEL_STRING_VAL = 0 */
> > -                     RTE_TEL_ARRAY_INT,    /* RTE_TEL_INT_VAL = 1 */
> > -                     RTE_TEL_ARRAY_U64,    /* RTE_TEL_u64_VAL = 2 */
> > -                     RTE_TEL_ARRAY_CONTAINER, /* RTE_TEL_CONTAINER = 3 */
> > +             [RTE_TEL_STRING_VAL] = RTE_TEL_ARRAY_STRING,
> > +             [RTE_TEL_INT_VAL] = RTE_TEL_ARRAY_INT,
> > +             [RTE_TEL_U64_VAL] = RTE_TEL_ARRAY_U64,
> > +             [RTE_TEL_CONTAINER] = RTE_TEL_ARRAY_CONTAINER,
> > +             [RTE_TEL_BOOL_VAL] = RTE_TEL_ARRAY_BOOL,
> >       };
>
> Really like this change!

I don't remember if this form is related to some CXX standard... but I
see that no compiler complained in the CI.


>
> >       d->type = array_types[type];
> >       d->data_len = 0;
> > @@ -80,6 +81,17 @@ rte_tel_data_add_array_u64(struct rte_tel_data *d, uint64_t x)
> >       return 0;
> >  }
> >
> > +int
> > +rte_tel_data_add_array_bool(struct rte_tel_data *d, bool x)
> > +{
> > +     if (d->type != RTE_TEL_ARRAY_BOOL)
> > +             return -EINVAL;
> > +     if (d->data_len >= RTE_TEL_MAX_ARRAY_ENTRIES)
> > +             return -ENOSPC;
> > +     d->data.array[d->data_len++].boolval = x;
> > +     return 0;
> > +}
> > +
> >  int
> >  rte_tel_data_add_array_container(struct rte_tel_data *d,
> >               struct rte_tel_data *val, int keep)
> <snip>
> > diff --git a/lib/telemetry/telemetry_json.h b/lib/telemetry/telemetry_json.h
> > index e3fae7c30d..c97da97366 100644
> > --- a/lib/telemetry/telemetry_json.h
> > +++ b/lib/telemetry/telemetry_json.h
> > @@ -7,6 +7,7 @@
> >
> >  #include <inttypes.h>
> >  #include <stdarg.h>
> > +#include <stdbool.h>
> >  #include <stdio.h>
> >  #include <rte_common.h>
> >  #include <rte_telemetry.h>
> > @@ -159,6 +160,21 @@ rte_tel_json_add_array_u64(char *buf, const int len, const int used,
> >       return ret == 0 ? used : end + ret;
> >  }
> >
> > +/* Appends a boolean into the JSON array in the provided buffer. */
> > +static inline int
> > +rte_tel_json_add_array_bool(char *buf, const int len, const int used,
> > +             bool val)
> > +{
> > +     int ret, end = used - 1; /* strip off final delimiter */
> > +     if (used <= 2) /* assume empty, since minimum is '[]' */
> > +             return __json_snprintf(buf, len, "[%s]",
> > +                             val ? "true" : "false");
> > +
> > +     ret = __json_snprintf(buf + end, len - end, ",%s]",
> > +                     val ? "true" : "false");
>
> Wonder if it's worthwhile doing a macro for this conditional, since the
> same ternary-operator snippet appears 4 times in this code.

Err, naming it would be hard and I don't see for now how we could reuse it.


>
> > +     return ret == 0 ? used : end + ret;
> > +}
> > +
> >  /*
> >   * Add a new element with raw JSON value to the JSON array stored in the
> >   * provided buffer.


--
David Marchand


  parent reply	other threads:[~2022-10-19 14:29 UTC|newest]

Thread overview: 9+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2022-10-19  7:37 David Marchand
2022-10-19  8:13 ` Morten Brørup
2022-10-19 13:47 ` Bruce Richardson
2022-10-19 14:18   ` Morten Brørup
2022-10-19 14:28   ` David Marchand [this message]
2022-10-19 16:47     ` Bruce Richardson
2022-10-19 18:08       ` Morten Brørup
2022-10-20  9:54 ` Power, Ciara
2022-10-20 11:48   ` David Marchand

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=CAJFAV8z+en7A1n4RLpooHWLFe4wpJVyk2Q-0_SHOm-U_MgEKOg@mail.gmail.com \
    --to=david.marchand@redhat.com \
    --cc=bruce.richardson@intel.com \
    --cc=ciara.power@intel.com \
    --cc=dev@dpdk.org \
    --cc=mb@smartsharesystems.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).