Re: [PATCH V7 11/29] qapi: list utility functions

[Steven Sistare]

On 3/9/2022 9:11 AM, Marc-André Lureau wrote:
> Hi
> 
> On Wed, Dec 22, 2021 at 11:42 PM Steve Sistare <steven.sistare@oracle.com 
> <mailto:steven.sistare@oracle.com>> wrote:
> 
>     Generalize strList_from_comma_list() to take any delimiter character, 
> rename
>     as strList_from_string(), and move it to qapi/util.c.  Also add
>     strv_from_strList() and QAPI_LIST_LENGTH().
> 
> Looks like you could easily split, and add some tests.

Will do.  
I don't see any tests that include qapi/util.h, so this will be a new test file.

For the split, how about:
  patch: qapi: strList_from_string
  patch: qapi: strv_from_strList
  patch: qapi: QAPI_LIST_LENGTH
  patch: qapi: unit tests for lists

Or do you prefer that unit tests be pushed with each function's patch?

>     No functional change.
> 
>     Signed-off-by: Steve Sistare <steven.sistare@oracle.com 
> <mailto:steven.sistare@oracle.com>>
>     ---
>      include/qapi/util.h | 28 ++++++++++++++++++++++++++++
>      monitor/hmp-cmds.c  | 29 ++---------------------------
>      qapi/qapi-util.c    | 37 +++++++++++++++++++++++++++++++++++++
>      3 files changed, 67 insertions(+), 27 deletions(-)
> 
>     diff --git a/include/qapi/util.h b/include/qapi/util.h
>     index 81a2b13..c249108 100644
>     --- a/include/qapi/util.h
>     +++ b/include/qapi/util.h
>     @@ -22,6 +22,8 @@ typedef struct QEnumLookup {
>          const int size;
>      } QEnumLookup;
> 
>     +struct strList;
>     +
>      const char *qapi_enum_lookup(const QEnumLookup *lookup, int val);
>      int qapi_enum_parse(const QEnumLookup *lookup, const char *buf,
>                          int def, Error **errp);
>     @@ -31,6 +33,19 @@ bool qapi_bool_parse(const char *name, const char 
> *value, bool *obj,
>      int parse_qapi_name(const char *name, bool complete);
> 
>      /*
>     + * Produce and return a NULL-terminated array of strings from @args.
>     + * All strings are g_strdup'd.
>     + */
>     +char **strv_from_strList(const struct strList *args);
> 
>     +
> 
> I'd suggest to use the dedicated glib type GStrv

Will do, here and in related code.

- Steve

>     +/*
>     + * Produce a strList from the character delimited string @in.
>     + * All strings are g_strdup'd.
>     + * A NULL or empty input string returns NULL.
>     + */
>     +struct strList *strList_from_string(const char *in, char delim);
>     +
>     +/*
>       * For any GenericList @list, insert @element at the front.
>       *
>       * Note that this macro evaluates @element exactly once, so it is safe
>     @@ -56,4 +71,17 @@ int parse_qapi_name(const char *name, bool complete);
>          (tail) = &(*(tail))->next; \
>      } while (0)
> 
>     +/*
>     + * For any GenericList @list, return its length.
>     + */
>     +#define QAPI_LIST_LENGTH(list) \
>     +    ({ \
>     +        int len = 0; \
>     +        typeof(list) elem; \
>     +        for (elem = list; elem != NULL; elem = elem->next) { \
>     +            len++; \
>     +        } \
>     +        len; \
>     +    })
>     +
>      #endif
>     diff --git a/monitor/hmp-cmds.c b/monitor/hmp-cmds.c
>     index b8c22da..5ca8b4b 100644
>     --- a/monitor/hmp-cmds.c
>     +++ b/monitor/hmp-cmds.c
>     @@ -43,6 +43,7 @@
>      #include "qapi/qapi-commands-run-state.h"
>      #include "qapi/qapi-commands-tpm.h"
>      #include "qapi/qapi-commands-ui.h"
>     +#include "qapi/util.h"
>      #include "qapi/qapi-visit-net.h"
>      #include "qapi/qapi-visit-migration.h"
>      #include "qapi/qmp/qdict.h"
>     @@ -70,32 +71,6 @@ bool hmp_handle_error(Monitor *mon, Error *err)
>          return false;
>      }
> 
>     -/*
>     - * Produce a strList from a comma separated list.
>     - * A NULL or empty input string return NULL.
>     - */
>     -static strList *strList_from_comma_list(const char *in)
>     -{
>     -    strList *res = NULL;
>     -    strList **tail = &res;
>     -
>     -    while (in && in[0]) {
>     -        char *comma = strchr(in, ',');
>     -        char *value;
>     -
>     -        if (comma) {
>     -            value = g_strndup(in, comma - in);
>     -            in = comma + 1; /* skip the , */
>     -        } else {
>     -            value = g_strdup(in);
>     -            in = NULL;
>     -        }
>     -        QAPI_LIST_APPEND(tail, value);
>     -    }
>     -
>     -    return res;
>     -}
>     -
>      void hmp_info_name(Monitor *mon, const QDict *qdict)
>      {
>          NameInfo *info;
>     @@ -1103,7 +1078,7 @@ void hmp_announce_self(Monitor *mon, const QDict 
> *qdict)
>                                                  migrate_announce_params());
> 
>          qapi_free_strList(params->interfaces);
>     -    params->interfaces = strList_from_comma_list(interfaces_str);
>     +    params->interfaces = strList_from_string(interfaces_str, ',');
>          params->has_interfaces = params->interfaces != NULL;
>          params->id = g_strdup(id);
>          params->has_id = !!params->id;
>     diff --git a/qapi/qapi-util.c b/qapi/qapi-util.c
>     index fda7044..edd51b3 100644
>     --- a/qapi/qapi-util.c
>     +++ b/qapi/qapi-util.c
>     @@ -15,6 +15,7 @@
>      #include "qapi/error.h"
>      #include "qemu/ctype.h"
>      #include "qapi/qmp/qerror.h"
>     +#include "qapi/qapi-builtin-types.h"
> 
>      CompatPolicy compat_policy;
> 
>     @@ -152,3 +153,39 @@ int parse_qapi_name(const char *str, bool complete)
>          }
>          return p - str;
>      }
>     +
>     +char **strv_from_strList(const strList *args)
>     +{
>     +    const strList *arg;
>     +    int i = 0;
>     +    char **argv = g_malloc((QAPI_LIST_LENGTH(args) + 1) * sizeof(char 
> *));
>     +
>     +    for (arg = args; arg != NULL; arg = arg->next) {
>     +        argv[i++] = g_strdup(arg->value);
>     +    }
>     +    argv[i] = NULL;
>     +
>     +    return argv;
>     +}
>     +
>     +strList *strList_from_string(const char *in, char delim)
>     +{
>     +    strList *res = NULL;
>     +    strList **tail = &res;
>     +
>     +    while (in && in[0]) {
>     +        char *next = strchr(in, delim);
>     +        char *value;
>     +
>     +        if (next) {
>     +            value = g_strndup(in, next - in);
>     +            in = next + 1; /* skip the delim */
>     +        } else {
>     +            value = g_strdup(in);
>     +            in = NULL;
>     +        }
>     +        QAPI_LIST_APPEND(tail, value);
>     +    }
>     +
>     +    return res;
>     +}
>     -- 
>     1.8.3.1
> 
> 
> 
> 
> -- 
> Marc-André Lureau