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

[Markus Armbruster]

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'm also fine with generalizing the function to better serve new uses.