Re: [PATCH V4 1/5] util: strList_from_string

[Steven Sistare <steven.sistare@oracle.com>]

On 2/21/2024 12:01 PM, Steven Sistare wrote:
> On 2/21/2024 8:29 AM, Markus Armbruster wrote:
>> I apologize for the lateness of my review.
> 
> No problem.  Thanks for the review.
> 
>> Steve Sistare <steven.sistare@oracle.com> writes:
>>
>>> Generalize hmp_split_at_comma() to take any delimiter string, rename
>>> as strList_from_string(), and move it to util/strList.c.
>>>
>>> No functional change.
>>>
>>> Signed-off-by: Steve Sistare <steven.sistare@oracle.com>
>>
>> I can't see an actual use of generalized delimiters outside tests in
>> this series.  Do you have uses?
> 
> In this series, it is called from hmp_announce_self and stats_filter; 
> those were formerly calls to hmp_split_at_comma.
> 
> In my live update cpr-exec series, there is an additional call site, with a
> space delimiter instead of comma.  Live update V9 is posted but is old and obsolete.  
> I will post V10 soon, but I am hoping you can pull this series first, so I can 
> whittle down my pending patches and omit these from V10.
> 
>>> ---
>>>  include/monitor/hmp.h  |  1 -
>>>  include/qemu/strList.h | 24 ++++++++++++++++++++++++
>>>  monitor/hmp-cmds.c     | 19 -------------------
>>>  net/net-hmp-cmds.c     |  3 ++-
>>>  stats/stats-hmp-cmds.c |  3 ++-
>>>  util/meson.build       |  1 +
>>>  util/strList.c         | 24 ++++++++++++++++++++++++
>>>  7 files changed, 53 insertions(+), 22 deletions(-)
>>>  create mode 100644 include/qemu/strList.h
>>>  create mode 100644 util/strList.c
>>>
>>> diff --git a/include/monitor/hmp.h b/include/monitor/hmp.h
>>> index 13f9a2d..2df661e 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/qemu/strList.h b/include/qemu/strList.h
>>> new file mode 100644
>>> index 0000000..010237f
>>> --- /dev/null
>>> +++ b/include/qemu/strList.h
>>> @@ -0,0 +1,24 @@
>>> +/*
>>> + * Copyright (c) 2022 - 2024 Oracle and/or its affiliates.
>>> + *
>>> + * This work is licensed under the terms of the GNU GPL, version 2 or later.
>>> + * See the COPYING file in the top-level directory.
>>> + */
>>> +
>>> +#ifndef QEMU_STR_LIST_H
>>> +#define QEMU_STR_LIST_H
>>> +
>>> +#include "qapi/qapi-builtin-types.h"
>>> +
>>> +/*
>>> + * Break @in into a strList using the delimiter string @delim.
>>> + * The delimiter is not included in the result.
>>> + * Return NULL if @in is NULL or an empty string.
>>> + * A leading, trailing, or consecutive delimiter produces an
>>> + * empty string at that position in the output.
>>> + * All strings are g_strdup'd, and the result can be freed
>>> + * using qapi_free_strList.
>>> + */
>>> +strList *strList_from_string(const char *in, const char *delim);
>>
>> The function name no longer tells us explicitly what the function does:
>> splitting the string.
> 
> The first sentence does not say it?
>   "Break @in into a strList using the delimiter string @delim"
> 
> Would you prefer this?
>   "Split string @in into a strList using the delimiter string @delim"

Sorry, I read your comment too quickly.  You want a different function name.
I propose:
  strList *str_split(const char *str, const char *delim);

- Steve