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

[Juan Quintela]

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

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.

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

>>                                              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.

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.

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

Agreed.

Later, Juan.