| Message ID | 20230208093600.242665-2-het.gala@nutanix.com |
|---|---|
| State | New |
| Headers | show |
| Series | migration: Modified 'migrate' QAPI command for migration | expand |
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. 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". 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. > 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. https://gcc.gnu.org/onlinedocs/gcc/Conditionals.html Besides, please don't mix code motion with code changes. [...]
On Wed, Feb 08, 2023 at 09:35:55AM +0000, Het Gala wrote: > renamed hmp_split_at_comma() --> str_split_at_comma() > Shifted helper function to qapi-util.c file. Give external linkage, as > this function will be handy in coming commit for migration. > > Minor correction: > g_strsplit(str ?: "", ",", -1) --> g_strsplit(str ? str : "", ",", -1) > > Suggested-by: Daniel P. Berrange <berrange@redhat.com> > Suggested-by: Manish Mishra <manish.mishra@nutanix.com> > Suggested-by: Aravind Retnakaran <aravind.retnakaran@nutanix.com> > Signed-off-by: Het Gala <het.gala@nutanix.com> > --- > include/monitor/hmp.h | 1 - > include/qapi/util.h | 1 + > monitor/hmp-cmds.c | 19 ------------------- > net/net-hmp-cmds.c | 2 +- > qapi/qapi-util.c | 19 +++++++++++++++++++ > stats/stats-hmp-cmds.c | 2 +- > 6 files changed, 22 insertions(+), 22 deletions(-) I expect this patch can be dropped, since I don't believe it is correct to be using it in patch 2. I left comments in that other patch with more details. With regards, Daniel
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.
On 09/02/23 5:30 pm, Markus Armbruster 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. > > 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". > > 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. Yes Markus, I agree with you. But we are also supporting 'uri':'str' right now for backward compatibility. So splitting of string might have needed. But I misunderstood a crucial part of exec string, and hence had to introduce this patch in the first place. Daniel made my understanding clear now in the 4th patch. Now, there is no need to introduce this patch. In the upcoming patchset version, this patch will be dropped. >> 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. > > https://urldefense.proofpoint.com/v2/url?u=https-3A__gcc.gnu.org_onlinedocs_gcc_Conditionals.html&d=DwIBAg&c=s883GpUCOChKOHiocYtGcg&r=-qwZZzrw4EKSsq0BK7MBd3wW1WEpXmJeng3ZUT5uBCg&m=gAsWrkiPm3MCpqkWQzGYo9-M2_2bkxfDAGmW8lgXmOAnW2YoDs5AhxGPt-Sc5xI3&s=Onmed5Fm0PImk6PD44bAvu8yQDrGuYU44yRYAw3Abjs&e= Ack. Thankyou for getting it into my knowledge. Will take care from next time > Besides, please don't mix code motion with code changes. Yes sure Markus. I apologise for not knowing it earlier and introducing such a patch, but lesson learned :) > [...] Regards, Het Gala
On 09/02/23 5:32 pm, Daniel P. Berrangé wrote: > On Wed, Feb 08, 2023 at 09:35:55AM +0000, Het Gala wrote: >> renamed hmp_split_at_comma() --> str_split_at_comma() >> Shifted helper function to qapi-util.c file. Give external linkage, as >> this function will be handy in coming commit for migration. >> >> Minor correction: >> g_strsplit(str ?: "", ",", -1) --> g_strsplit(str ? str : "", ",", -1) >> >> Suggested-by: Daniel P. Berrange <berrange@redhat.com> >> Suggested-by: Manish Mishra <manish.mishra@nutanix.com> >> Suggested-by: Aravind Retnakaran <aravind.retnakaran@nutanix.com> >> Signed-off-by: Het Gala <het.gala@nutanix.com> >> --- >> include/monitor/hmp.h | 1 - >> include/qapi/util.h | 1 + >> monitor/hmp-cmds.c | 19 ------------------- >> net/net-hmp-cmds.c | 2 +- >> qapi/qapi-util.c | 19 +++++++++++++++++++ >> stats/stats-hmp-cmds.c | 2 +- >> 6 files changed, 22 insertions(+), 22 deletions(-) > I expect this patch can be dropped, since I don't believe it is > correct to be using it in patch 2. I left comments in that other > patch with more details. Yes Daniel. This patch will be dropped in upcoming version of this patchset. > With regards, > Daniel Regards, Het Gala
On 09/02/23 5:42 pm, Juan Quintela wrote: > 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://urldefense.proofpoint.com/v2/url?u=https-3A__gcc.gnu.org_onlinedocs_gcc_Conditionals.html&d=DwIBAg&c=s883GpUCOChKOHiocYtGcg&r=-qwZZzrw4EKSsq0BK7MBd3wW1WEpXmJeng3ZUT5uBCg&m=b-y7IKTlkPluPqpf6lI-BKMLDSrV5JJRRzU39eSq6CpslAITuH5Cxi6l_XugJfkM&s=Z1FND19C0lNnL8v7t_pifjUxyCxbHC8OL2fX-euPRb4&e= >> >> Besides, please don't mix code motion with code changes. > Agreed. Thankyou for your comments Juan. After discussing on the same with Daniel, this patch will be dropped in the next patchset version as the str_split func. it would not be necessary in the first place. > Later, Juan. Regards, Het Gala
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.
diff --git a/include/monitor/hmp.h b/include/monitor/hmp.h index 2220f14fc9..e80848fbd0 100644 --- a/include/monitor/hmp.h +++ b/include/monitor/hmp.h @@ -19,7 +19,6 @@ bool hmp_handle_error(Monitor *mon, Error *err); void hmp_help_cmd(Monitor *mon, const char *name); -strList *hmp_split_at_comma(const char *str); void hmp_info_name(Monitor *mon, const QDict *qdict); void hmp_info_version(Monitor *mon, const QDict *qdict); diff --git a/include/qapi/util.h b/include/qapi/util.h index 81a2b13a33..6c8d8575e3 100644 --- a/include/qapi/util.h +++ b/include/qapi/util.h @@ -29,6 +29,7 @@ bool qapi_bool_parse(const char *name, const char *value, bool *obj, Error **errp); int parse_qapi_name(const char *name, bool complete); +struct strList *str_split_at_comma(const char *str); /* * For any GenericList @list, insert @element at the front. diff --git a/monitor/hmp-cmds.c b/monitor/hmp-cmds.c index 34bd8c67d7..9665e6e0a5 100644 --- a/monitor/hmp-cmds.c +++ b/monitor/hmp-cmds.c @@ -39,25 +39,6 @@ bool hmp_handle_error(Monitor *mon, Error *err) return false; } -/* - * Split @str at comma. - * A null @str defaults to "". - */ -strList *hmp_split_at_comma(const char *str) -{ - char **split = g_strsplit(str ?: "", ",", -1); - 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; -} - void hmp_info_name(Monitor *mon, const QDict *qdict) { NameInfo *info; diff --git a/net/net-hmp-cmds.c b/net/net-hmp-cmds.c index 41d326bf5f..a3c597a727 100644 --- a/net/net-hmp-cmds.c +++ b/net/net-hmp-cmds.c @@ -72,7 +72,7 @@ void hmp_announce_self(Monitor *mon, const QDict *qdict) migrate_announce_params()); qapi_free_strList(params->interfaces); - params->interfaces = hmp_split_at_comma(interfaces_str); + params->interfaces = str_split_at_comma(interfaces_str); params->has_interfaces = params->interfaces != NULL; params->id = g_strdup(id); qmp_announce_self(params, NULL); diff --git a/qapi/qapi-util.c b/qapi/qapi-util.c index 63596e11c5..e26b9d957b 100644 --- a/qapi/qapi-util.c +++ b/qapi/qapi-util.c @@ -152,3 +152,22 @@ int parse_qapi_name(const char *str, bool complete) } return p - str; } + +/* + * Split @str at comma. + * A null @str defaults to "". + */ +strList *str_split_at_comma(const char *str) +{ + char **split = g_strsplit(str ? str : "", ",", -1); + 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; +} diff --git a/stats/stats-hmp-cmds.c b/stats/stats-hmp-cmds.c index 531e35d128..cfee05a076 100644 --- a/stats/stats-hmp-cmds.c +++ b/stats/stats-hmp-cmds.c @@ -174,7 +174,7 @@ static StatsFilter *stats_filter(StatsTarget target, const char *names, request->provider = provider_idx; if (names && !g_str_equal(names, "*")) { request->has_names = true; - request->names = hmp_split_at_comma(names); + request->names = str_split_at_comma(names); } QAPI_LIST_PREPEND(request_list, request); }
renamed hmp_split_at_comma() --> str_split_at_comma() Shifted helper function to qapi-util.c file. Give external linkage, as this function will be handy in coming commit for migration. Minor correction: g_strsplit(str ?: "", ",", -1) --> g_strsplit(str ? str : "", ",", -1) Suggested-by: Daniel P. Berrange <berrange@redhat.com> Suggested-by: Manish Mishra <manish.mishra@nutanix.com> Suggested-by: Aravind Retnakaran <aravind.retnakaran@nutanix.com> Signed-off-by: Het Gala <het.gala@nutanix.com> --- include/monitor/hmp.h | 1 - include/qapi/util.h | 1 + monitor/hmp-cmds.c | 19 ------------------- net/net-hmp-cmds.c | 2 +- qapi/qapi-util.c | 19 +++++++++++++++++++ stats/stats-hmp-cmds.c | 2 +- 6 files changed, 22 insertions(+), 22 deletions(-)