Re: [PATCH v2 1/6] migration: moved hmp_split_at_commma() helper func to qapi-util.c file

[Markus Armbruster]

Juan Quintela <quintela@redhat.com> writes:

> Markus Armbruster <armbru@redhat.com> wrote:
>> Het Gala <het.gala@nutanix.com> writes:
>>
>>> renamed hmp_split_at_comma() --> str_split_at_comma()
>>> Shifted helper function to qapi-util.c file.
>>
>> Not an appropriate home.
>
> I don't have an opinion here.
>
>> If we have to split up a string in QAPI/QMP, we screwed up.  Let me
>> explain.
>>
>> In QAPI/QMP, data is not to be encoded in strings, it is to be
>> represented directly in JSON.  Thus, ["a", "b", "c"], *not* "a,b,c".
>
> In this case, we are already screwed up O:-)

A loooong time ago :)

> the uri value in migration has to be split.
> What this series does is creating a new way to express the command
> (something on the lines that you describe), but we still have to
> maintain the old way of doing it for some time, so we need this
> function.

And that's fine.  I just want it to stay out of qapi/, to avoid giving
people the idea that splitting string is something QAPI wants you to do.

>> When you find yourself parsing QAPI/QMP string values, you're dealing
>> with a case where we violated this interface design principle.  Happens,
>> but the proper home for code helping to deal with this is *not* qapi/.
>> Simply because QAPI is not about parsing strings.
>
> Ok, I drop my review-by.
>
> See why I was asking for reviews from QAPI libvirt folks for this code O:-)

Better late than never (I hope).

>>>                                              Give external linkage, as
>>> this function will be handy in coming commit for migration.
>>
>> It already has external linkage.
>>
>>> Minor correction:
>>> g_strsplit(str ?: "", ",", -1) --> g_strsplit(str ? str : "", ",", -1)
>>
>> This is not actually a correction :)
>>
>> Omitting the second operand of a conditional expression like x ?: y is
>> equivalent to x ? x : y, except it evaluates x only once.
>
> You are (way) more C layer than me.

Guilty as charged.

> Once told that, I think that what he wanted to do is making this c
> standard, no gcc standard.
>
> Once told that, I think that every C compiler worth its salt has this
> extension.

There are hundreds of uses in the tree.

>> https://gcc.gnu.org/onlinedocs/gcc/Conditionals.html
>>
>> Besides, please don't mix code motion with code changes.
>
> Agreed.
>
> Later, Juan.