Re: [PATCH V2 1/4] qapi: strList_from_string

[Alex Bennée]

Steven Sistare <steven.sistare@oracle.com> writes:

> On 2/8/2023 1:43 AM, Marc-André Lureau wrote:
>> Hi
>> 
>> On Tue, Feb 7, 2023 at 10:50 PM Steve Sistare <steven.sistare@oracle.com> 
>> wrote:
>>>
>>> Generalize hmp_split_at_comma() to take any delimiter character, rename
>>> as strList_from_string(), and move it to qapi/util.c.
>>>
>>> No functional change.
>> 
>> The g_strsplit() version was a bit simpler, but if you want to
>> optimize it a bit for 1 char delimiter only, ok.
>> 
>> Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
>
> Yes, and it saves a malloc+free for the array.  Small stuff, but I
> thought it worth a few lines of code.  Thanks for the speedy review!

But is the HMP path that performance critical? Otherwise I'd favour
consistent use of the glib APIs because its one less thing to get wrong.

>
> - Steve
>
>>> Signed-off-by: Steve Sistare <steven.sistare@oracle.com>
>>> ---
>>>  include/monitor/hmp.h  |  1 -
>>>  include/qapi/util.h    |  9 +++++++++
>>>  monitor/hmp-cmds.c     | 19 -------------------
>>>  net/net-hmp-cmds.c     |  2 +-
>>>  qapi/qapi-util.c       | 23 +++++++++++++++++++++++
>>>  stats/stats-hmp-cmds.c |  2 +-
>>>  6 files changed, 34 insertions(+), 22 deletions(-)
>>>
>>> diff --git a/include/monitor/hmp.h b/include/monitor/hmp.h
>>> index 2220f14..e80848f 100644
>>> --- a/include/monitor/hmp.h
>>> +++ b/include/monitor/hmp.h
>>> @@ -19,7 +19,6 @@
>>>
>>>  bool hmp_handle_error(Monitor *mon, Error *err);
>>>  void hmp_help_cmd(Monitor *mon, const char *name);
>>> -strList *hmp_split_at_comma(const char *str);
>>>
>>>  void hmp_info_name(Monitor *mon, const QDict *qdict);
>>>  void hmp_info_version(Monitor *mon, const QDict *qdict);
>>> diff --git a/include/qapi/util.h b/include/qapi/util.h
>>> index 81a2b13..7d88b09 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,13 @@ bool qapi_bool_parse(const char *name, const char 
>>> *value, bool *obj,
>>>  int parse_qapi_name(const char *name, bool complete);
>>>
>>>  /*
>>> + * 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
>>> diff --git a/monitor/hmp-cmds.c b/monitor/hmp-cmds.c
>>> index 34bd8c6..9665e6e 100644
>>> --- a/monitor/hmp-cmds.c
>>> +++ b/monitor/hmp-cmds.c
>>> @@ -39,25 +39,6 @@ bool hmp_handle_error(Monitor *mon, Error *err)
>>>      return false;
>>>  }
>>>
>>> -/*
>>> - * Split @str at comma.
>>> - * A null @str defaults to "".
>>> - */
>>> -strList *hmp_split_at_comma(const char *str)
>>> -{
>>> -    char **split = g_strsplit(str ?: "", ",", -1);
>>> -    strList *res = NULL;
>>> -    strList **tail = &res;
>>> -    int i;
>>> -
>>> -    for (i = 0; split[i]; i++) {
>>> -        QAPI_LIST_APPEND(tail, split[i]);
>>> -    }
>>> -
>>> -    g_free(split);
>>> -    return res;
>>> -}
>>> -
>>>  void hmp_info_name(Monitor *mon, const QDict *qdict)
>>>  {
>>>      NameInfo *info;
>>> diff --git a/net/net-hmp-cmds.c b/net/net-hmp-cmds.c
>>> index 41d326b..30422d9 100644
>>> --- a/net/net-hmp-cmds.c
>>> +++ b/net/net-hmp-cmds.c
>>> @@ -72,7 +72,7 @@ void hmp_announce_self(Monitor *mon, const QDict *qdict)
>>>                                              migrate_announce_params());
>>>
>>>      qapi_free_strList(params->interfaces);
>>> -    params->interfaces = hmp_split_at_comma(interfaces_str);
>>> +    params->interfaces = strList_from_string(interfaces_str, ',');
>>>      params->has_interfaces = params->interfaces != NULL;
>>>      params->id = g_strdup(id);
>>>      qmp_announce_self(params, NULL);
>>> diff --git a/qapi/qapi-util.c b/qapi/qapi-util.c
>>> index 63596e1..b61c73c 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,25 @@ int parse_qapi_name(const char *str, bool complete)
>>>      }
>>>      return p - str;
>>>  }
>>> +
>>> +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;
>>> +}
>>> diff --git a/stats/stats-hmp-cmds.c b/stats/stats-hmp-cmds.c
>>> index 531e35d..4a2adf8 100644
>>> --- a/stats/stats-hmp-cmds.c
>>> +++ b/stats/stats-hmp-cmds.c
>>> @@ -174,7 +174,7 @@ static StatsFilter *stats_filter(StatsTarget target, 
>>> const char *names,
>>>              request->provider = provider_idx;
>>>              if (names && !g_str_equal(names, "*")) {
>>>                  request->has_names = true;
>>> -                request->names = hmp_split_at_comma(names);
>>> +                request->names = strList_from_string(names, ',');
>>>              }
>>>              QAPI_LIST_PREPEND(request_list, request);
>>>          }
>>> --
>>> 1.8.3.1
>>>
>>>
>> 
>> 


-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro