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

[Steven Sistare]

On 6/13/2023 8:33 AM, Markus Armbruster wrote:
> Steven Sistare <steven.sistare@oracle.com> writes:
>> On 2/10/2023 4:25 AM, Markus Armbruster wrote:
>>> Steven Sistare <steven.sistare@oracle.com> writes:
>>>> On 2/9/2023 1:59 PM, Markus Armbruster wrote:
>>>>> Steven Sistare <steven.sistare@oracle.com> writes:
>>>>>> On 2/9/2023 11:46 AM, Markus Armbruster wrote:
>>>>>>> Steven Sistare <steven.sistare@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.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.
>>>>>
>>>>> 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