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

[Markus Armbruster]

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

> On 2/9/2023 1:59 PM, Markus Armbruster wrote:
>> Steven Sistare <steven.sistare@oracle.com> writes:
>>> On 2/9/2023 11:46 AM, Markus Armbruster wrote:
>>>> Steven Sistare <steven.sistare@oracle.com> writes:

[...]

>>>>> For more context, this patch has been part of my larger series for live 
>>>>> update,
>>>>> and I am submitting this separately to reduce the size of that series and 
>>>>> make
>>>>> forward progress:
>>>>>     
>>>>> https://lore.kernel.org/qemu-devel/1658851843-236870-1-git-send-email-steven.sistare@oracle.com/
>>>>>
>>>>> In that series, strList_from_string is used to parse a space-separated 
>>>>> list of args
>>>>> in an HMP command, and pass them to the new qemu binary.
>>>>>     
>>>>> https://lore.kernel.org/qemu-devel/1658851843-236870-16-git-send-email-steven.sistare@oracle.com/
>>>>>
>>>>> I moved and renamed the generalized function because I thought it might 
>>>>> be useful
>>>>> to others in the future, along with the other functions in this 'string 
>>>>> list functions'
>>>>> patch series.  But if you disagree, I can minimally modify 
>>>>> hmp_split_at_comma() in its 
>>>>> current location.
>>>>
>>>> I'm fine with moving it out of monitor/ if there are uses outside the
>>>> monitor.  I just don't think qapi/ is the right home.
>>>
>>> I don't know where else it would go, as strList is a QAPI type.
>>> include/qapi/util.h already defines QAPI_LIST_PREPEND and QAPI_LIST_APPEND, 
>>> so it
>>> seems like the natural place to add qapi strList functions.  I am open to
>>> suggestions.
>> 
>> What about util/?  Plenty of QAPI use there already.
>> 
>> Another thought.  Current hmp_split_at_comma() does two things:
>> 
>>     strList *hmp_split_at_comma(const char *str)
>>     {
>> 
>> One, split a comma-separated string into NULL-terminated a dynamically
>> allocated char *[]:
>> 
>>         char **split = g_strsplit(str ?: "", ",", -1);
>> 
>> Two, convert a dynamically allocated char *[] into a strList:
>> 
>>         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;
>>     }
>> 
>> Part two could live in qapi/.
>
> Works for me.

Note that I'm not demanding such a split.  I'm merely throwing in
another idea for you to use or reject.

> For future reference, what is your organizing principle for putting things in 
> qapi/ vs util/ ?  I see plenty of calls to g_str* functions from qapi/*, so I 
> don't know why removing g_strsplit changes the answer.
>
> Per your principle, where does strv_from_strList (patch 3) belong?  And if I
> substitute char ** for GStrv, does the answer change?

As is, qapi/qapi-util provides:

1. Helpers for qapi/ and QAPI-generated code.  Some of them are
   used elsewhere, too.  That's fine.

2. Tools for working with QAPI data types such as GenericList.

strv_from_strList() would fall under 2.  Same if you use char **
instead.

hmp_split_at_comma() admittedly also falls under 2.  I just dislike
putting things under qapi/ that contradict QAPI design principles.

util/ is a bit of a grabbag, I feel.  Perhaps we could describe it as
"utilities that don't really fit into a particular subsystem".

Does this help you along?