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?
(and apologies for my long delay in continuing this conversation).
- 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?
>
