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

[Steven Sistare]

On 2/10/2023 4:25 AM, Markus Armbruster wrote:
> 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.

I decided to not split the function.  IMO having part 2 free memory allocated
by its caller is not clean.

However, I will base it on your original function, slightly modified:

strList *strList_from_string(const char *str, char *delim)
{
    g_autofree char **split = g_strsplit(str ?: "", delim, -1);
    strList *res = NULL;
    strList **tail = &res;

    for (; *split; split++) {
        QAPI_LIST_APPEND(tail, *split);
    }

    return res;
}

>> 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.

What design principle does strList_from_string contradict?  Are you OK with 
putting the simplified version shown above in qapi-util?

(and apologies for my long delay in continuing this conversation).

- Steve

> 
> 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?
>