On 6/13/2023 8:33 AM, Markus Armbruster wrote: > Steven Sistare <steven.sist...@oracle.com> writes: >> On 2/10/2023 4:25 AM, Markus Armbruster wrote: >>> Steven Sistare <steven.sist...@oracle.com> writes: >>>> 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: >>> >>> [...] >>> >>>>>>>> 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. >>> >>> Note that I'm not demanding such a split. I'm merely throwing in >>> another idea for you to use or reject. >> >> I decided to not split the function. IMO having part 2 free memory allocated >> by its caller is not clean. >> >> However, I will base it on your original function, slightly modified: >> >> strList *strList_from_string(const char *str, char *delim) >> { >> g_autofree char **split = g_strsplit(str ?: "", delim, -1); >> strList *res = NULL; >> strList **tail = &res; >> >> for (; *split; split++) { >> QAPI_LIST_APPEND(tail, *split); >> } >> >> return res; >> } >> >>>> 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? >>> >>> As is, qapi/qapi-util provides: >>> >>> 1. Helpers for qapi/ and QAPI-generated code. Some of them are >>> used elsewhere, too. That's fine. >>> >>> 2. Tools for working with QAPI data types such as GenericList. >>> >>> strv_from_strList() would fall under 2. Same if you use char ** >>> instead. >>> >>> hmp_split_at_comma() admittedly also falls under 2. I just dislike >>> putting things under qapi/ that contradict QAPI design principles. >> >> What design principle does strList_from_string contradict? Are you OK with >> putting the simplified version shown above in qapi-util? > > The design principle is "use JSON to encode structured data as text in > QAPI/QMP". > > Do: "mumble": [1, 2, 3] > > Don't: "mumble": "1,2,3"
I don't mumble, but I sometimes mutter and ramble. > We violate the principle in a couple of places. Some are arguably > mistakes, some are pragmatic exceptions. > > The principle implies "the only parser QAPI needs is the JSON parser". > > By adding other parsers to QAPI, we send a misleading signal, namely > that encoding structured data in a way that requires parsing is okay. > It's not, generally. > > So, I'd prefer to find another home for code that splits strings at > comma / delimiter. > >> (and apologies for my long delay in continuing this conversation). > > I'm in no position to take offense there ;) Thanks, that makes it clear. I propose to move strList_from_string and strv_from_strList to new files util/strList.c and include/qemu/strList.h, and leave QAPI_LIST_LENGTH in include/qapi/util.h. (cutil.c already has string functions, but only uses simple C types, so not the best place to add the strList type). Sound OK? - Steve