On 2/9/2023 1:59 PM, Markus Armbruster wrote: > Steven Sistare <steven.sist...@oracle.com> writes: >> 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. > > 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
