On 2/9/2023 11:46 AM, Markus Armbruster wrote: > Steven Sistare <steven.sist...@oracle.com> writes: > >> On 2/9/2023 5:02 AM, Markus Armbruster wrote: >>> Alex Bennée <alex.ben...@linaro.org> writes: >>> >>>> Steven Sistare <steven.sist...@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.sist...@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.lur...@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.sist...@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.sist...@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. >
