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

[Markus Armbruster]

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"

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 ;)

> - Steve
>
>> 
>> util/ is a bit of a grabbag, I feel.  Perhaps we could describe it as
>> "utilities that don't really fit into a particular subsystem".
>> 
>> Does this help you along?