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

[Markus Armbruster]

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

>
> - Steve
>
>> 
>> I'm also fine with generalizing the function to better serve new uses.
>>