[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH V2 1/4] qapi: strList_from_string
From: |
Steven Sistare |
Subject: |
Re: [PATCH V2 1/4] qapi: strList_from_string |
Date: |
Thu, 9 Feb 2023 12:00:09 -0500 |
User-agent: |
Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:102.0) Gecko/20100101 Thunderbird/102.6.1 |
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.
- Steve
>
> I'm also fine with generalizing the function to better serve new uses.
>
- [PATCH V2 0/4] string list functions, Steve Sistare, 2023/02/07
- [PATCH V2 1/4] qapi: strList_from_string, Steve Sistare, 2023/02/07
- Re: [PATCH V2 1/4] qapi: strList_from_string, Marc-André Lureau, 2023/02/08
- Re: [PATCH V2 1/4] qapi: strList_from_string, Steven Sistare, 2023/02/08
- Re: [PATCH V2 1/4] qapi: strList_from_string, Alex Bennée, 2023/02/08
- Re: [PATCH V2 1/4] qapi: strList_from_string, Steven Sistare, 2023/02/08
- Re: [PATCH V2 1/4] qapi: strList_from_string, Markus Armbruster, 2023/02/09
- Re: [PATCH V2 1/4] qapi: strList_from_string, Steven Sistare, 2023/02/09
- Re: [PATCH V2 1/4] qapi: strList_from_string, Markus Armbruster, 2023/02/09
- Re: [PATCH V2 1/4] qapi: strList_from_string,
Steven Sistare <=
- Re: [PATCH V2 1/4] qapi: strList_from_string, Markus Armbruster, 2023/02/09
- Re: [PATCH V2 1/4] qapi: strList_from_string, Steven Sistare, 2023/02/09
- Re: [PATCH V2 1/4] qapi: strList_from_string, Markus Armbruster, 2023/02/10
[PATCH V2 3/4] qapi: strv_from_strList, Steve Sistare, 2023/02/07
[PATCH V2 4/4] qapi: strList unit tests, Steve Sistare, 2023/02/07
[PATCH V2 2/4] qapi: QAPI_LIST_LENGTH, Steve Sistare, 2023/02/07
Re: [PATCH V2 0/4] string list functions, Markus Armbruster, 2023/02/09
Re: [PATCH V2 0/4] string list functions, Daniel P . Berrangé, 2023/02/09