Juan Quintela <quint...@redhat.com> writes: > Markus Armbruster <arm...@redhat.com> wrote: >> Het Gala <het.g...@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.