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

[Steven Sistare]

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:
>>>
>>>> On 2/9/2023 5:02 AM, Markus Armbruster wrote:
>>>>> Alex Bennée <alex.bennee@linaro.org> writes:
>>>>>
>>>>>> 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.
>>>>>
>>>>> The patch reverts my recent commit 0d79271b570 "hmp: Rewrite
>>>>> strlist_from_comma_list() as hmp_split_at_comma()", with a different
>>>>> function name and place, and an additional parameter.
>>>>>
>>>>> There is no explanation for the revert.
>>>>>
>>>>> An intentional revert without even mentioning it would be uncourteous.
>>>>> I don't think this is the case here.  I figure you wrote this patch
>>>>> before you saw my commit, then rebased, keeping the old code.  A simple
>>>>> rebase mistake, easy enough to correct.
>>>>
>>>> Hi Markus, I am sorry, I intended no slight.
>>>
>>> Certainly no offense taken :)
>>>
>>>>                                               I will document your commit
>>>> in this commit message.  And in response to Alex' comment, I will use your
>>>> version as the basis of the new function.
>>>>
>>>> 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.

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?

- Steve