| Message ID | 1675795727-235010-2-git-send-email-steven.sistare@oracle.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | string list functions | expand |
Hi On Tue, Feb 7, 2023 at 10:50 PM Steve Sistare <steven.sistare@oracle.com> wrote: > > Generalize hmp_split_at_comma() to take any delimiter character, rename > as strList_from_string(), and move it to qapi/util.c. > > No functional change. The g_strsplit() version was a bit simpler, but if you want to optimize it a bit for 1 char delimiter only, ok. Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com> > > Signed-off-by: Steve Sistare <steven.sistare@oracle.com> > --- > include/monitor/hmp.h | 1 - > include/qapi/util.h | 9 +++++++++ > monitor/hmp-cmds.c | 19 ------------------- > net/net-hmp-cmds.c | 2 +- > qapi/qapi-util.c | 23 +++++++++++++++++++++++ > stats/stats-hmp-cmds.c | 2 +- > 6 files changed, 34 insertions(+), 22 deletions(-) > > diff --git a/include/monitor/hmp.h b/include/monitor/hmp.h > index 2220f14..e80848f 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 81a2b13..7d88b09 100644 > --- a/include/qapi/util.h > +++ b/include/qapi/util.h > @@ -22,6 +22,8 @@ typedef struct QEnumLookup { > const int size; > } QEnumLookup; > > +struct strList; > + > const char *qapi_enum_lookup(const QEnumLookup *lookup, int val); > int qapi_enum_parse(const QEnumLookup *lookup, const char *buf, > int def, Error **errp); > @@ -31,6 +33,13 @@ bool qapi_bool_parse(const char *name, const char *value, bool *obj, > int parse_qapi_name(const char *name, bool complete); > > /* > + * Produce a strList from the character delimited string @in. > + * All strings are g_strdup'd. > + * A NULL or empty input string returns NULL. > + */ > +struct strList *strList_from_string(const char *in, char delim); > + > +/* > * For any GenericList @list, insert @element at the front. > * > * Note that this macro evaluates @element exactly once, so it is safe > diff --git a/monitor/hmp-cmds.c b/monitor/hmp-cmds.c > index 34bd8c6..9665e6e 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 41d326b..30422d9 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 = strList_from_string(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 63596e1..b61c73c 100644 > --- a/qapi/qapi-util.c > +++ b/qapi/qapi-util.c > @@ -15,6 +15,7 @@ > #include "qapi/error.h" > #include "qemu/ctype.h" > #include "qapi/qmp/qerror.h" > +#include "qapi/qapi-builtin-types.h" > > CompatPolicy compat_policy; > > @@ -152,3 +153,25 @@ int parse_qapi_name(const char *str, bool complete) > } > return p - str; > } > + > +strList *strList_from_string(const char *in, char delim) > +{ > + strList *res = NULL; > + strList **tail = &res; > + > + while (in && in[0]) { > + char *next = strchr(in, delim); > + char *value; > + > + if (next) { > + value = g_strndup(in, next - in); > + in = next + 1; /* skip the delim */ > + } else { > + value = g_strdup(in); > + in = NULL; > + } > + QAPI_LIST_APPEND(tail, value); > + } > + > + return res; > +} > diff --git a/stats/stats-hmp-cmds.c b/stats/stats-hmp-cmds.c > index 531e35d..4a2adf8 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 = strList_from_string(names, ','); > } > QAPI_LIST_PREPEND(request_list, request); > } > -- > 1.8.3.1 > >
On 2/8/2023 1:43 AM, Marc-André Lureau wrote: > Hi > > On Tue, Feb 7, 2023 at 10:50 PM Steve Sistare <steven.sistare@oracle.com> wrote: >> >> Generalize hmp_split_at_comma() to take any delimiter character, rename >> as strList_from_string(), and move it to qapi/util.c. >> >> No functional change. > > The g_strsplit() version was a bit simpler, but if you want to > optimize it a bit for 1 char delimiter only, ok. > > Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com> Yes, and it saves a malloc+free for the array. Small stuff, but I thought it worth a few lines of code. Thanks for the speedy review! - Steve >> Signed-off-by: Steve Sistare <steven.sistare@oracle.com> >> --- >> include/monitor/hmp.h | 1 - >> include/qapi/util.h | 9 +++++++++ >> monitor/hmp-cmds.c | 19 ------------------- >> net/net-hmp-cmds.c | 2 +- >> qapi/qapi-util.c | 23 +++++++++++++++++++++++ >> stats/stats-hmp-cmds.c | 2 +- >> 6 files changed, 34 insertions(+), 22 deletions(-) >> >> diff --git a/include/monitor/hmp.h b/include/monitor/hmp.h >> index 2220f14..e80848f 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 81a2b13..7d88b09 100644 >> --- a/include/qapi/util.h >> +++ b/include/qapi/util.h >> @@ -22,6 +22,8 @@ typedef struct QEnumLookup { >> const int size; >> } QEnumLookup; >> >> +struct strList; >> + >> const char *qapi_enum_lookup(const QEnumLookup *lookup, int val); >> int qapi_enum_parse(const QEnumLookup *lookup, const char *buf, >> int def, Error **errp); >> @@ -31,6 +33,13 @@ bool qapi_bool_parse(const char *name, const char *value, bool *obj, >> int parse_qapi_name(const char *name, bool complete); >> >> /* >> + * Produce a strList from the character delimited string @in. >> + * All strings are g_strdup'd. >> + * A NULL or empty input string returns NULL. >> + */ >> +struct strList *strList_from_string(const char *in, char delim); >> + >> +/* >> * For any GenericList @list, insert @element at the front. >> * >> * Note that this macro evaluates @element exactly once, so it is safe >> diff --git a/monitor/hmp-cmds.c b/monitor/hmp-cmds.c >> index 34bd8c6..9665e6e 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 41d326b..30422d9 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 = strList_from_string(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 63596e1..b61c73c 100644 >> --- a/qapi/qapi-util.c >> +++ b/qapi/qapi-util.c >> @@ -15,6 +15,7 @@ >> #include "qapi/error.h" >> #include "qemu/ctype.h" >> #include "qapi/qmp/qerror.h" >> +#include "qapi/qapi-builtin-types.h" >> >> CompatPolicy compat_policy; >> >> @@ -152,3 +153,25 @@ int parse_qapi_name(const char *str, bool complete) >> } >> return p - str; >> } >> + >> +strList *strList_from_string(const char *in, char delim) >> +{ >> + strList *res = NULL; >> + strList **tail = &res; >> + >> + while (in && in[0]) { >> + char *next = strchr(in, delim); >> + char *value; >> + >> + if (next) { >> + value = g_strndup(in, next - in); >> + in = next + 1; /* skip the delim */ >> + } else { >> + value = g_strdup(in); >> + in = NULL; >> + } >> + QAPI_LIST_APPEND(tail, value); >> + } >> + >> + return res; >> +} >> diff --git a/stats/stats-hmp-cmds.c b/stats/stats-hmp-cmds.c >> index 531e35d..4a2adf8 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 = strList_from_string(names, ','); >> } >> QAPI_LIST_PREPEND(request_list, request); >> } >> -- >> 1.8.3.1 >> >> > >
Steven Sistare <steven.sistare@oracle.com> writes: > On 2/8/2023 1:43 AM, Marc-André Lureau wrote: >> Hi >> >> On Tue, Feb 7, 2023 at 10:50 PM Steve Sistare <steven.sistare@oracle.com> wrote: >>> >>> Generalize hmp_split_at_comma() to take any delimiter character, rename >>> as strList_from_string(), and move it to qapi/util.c. >>> >>> No functional change. >> >> The g_strsplit() version was a bit simpler, but if you want to >> optimize it a bit for 1 char delimiter only, ok. >> >> Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com> > > Yes, and it saves a malloc+free for the array. Small stuff, but I > thought it worth a few lines of code. Thanks for the speedy review! But is the HMP path that performance critical? Otherwise I'd favour consistent use of the glib APIs because its one less thing to get wrong. > > - Steve > >>> Signed-off-by: Steve Sistare <steven.sistare@oracle.com> >>> --- >>> include/monitor/hmp.h | 1 - >>> include/qapi/util.h | 9 +++++++++ >>> monitor/hmp-cmds.c | 19 ------------------- >>> net/net-hmp-cmds.c | 2 +- >>> qapi/qapi-util.c | 23 +++++++++++++++++++++++ >>> stats/stats-hmp-cmds.c | 2 +- >>> 6 files changed, 34 insertions(+), 22 deletions(-) >>> >>> diff --git a/include/monitor/hmp.h b/include/monitor/hmp.h >>> index 2220f14..e80848f 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 81a2b13..7d88b09 100644 >>> --- a/include/qapi/util.h >>> +++ b/include/qapi/util.h >>> @@ -22,6 +22,8 @@ typedef struct QEnumLookup { >>> const int size; >>> } QEnumLookup; >>> >>> +struct strList; >>> + >>> const char *qapi_enum_lookup(const QEnumLookup *lookup, int val); >>> int qapi_enum_parse(const QEnumLookup *lookup, const char *buf, >>> int def, Error **errp); >>> @@ -31,6 +33,13 @@ bool qapi_bool_parse(const char *name, const char *value, bool *obj, >>> int parse_qapi_name(const char *name, bool complete); >>> >>> /* >>> + * Produce a strList from the character delimited string @in. >>> + * All strings are g_strdup'd. >>> + * A NULL or empty input string returns NULL. >>> + */ >>> +struct strList *strList_from_string(const char *in, char delim); >>> + >>> +/* >>> * For any GenericList @list, insert @element at the front. >>> * >>> * Note that this macro evaluates @element exactly once, so it is safe >>> diff --git a/monitor/hmp-cmds.c b/monitor/hmp-cmds.c >>> index 34bd8c6..9665e6e 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 41d326b..30422d9 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 = strList_from_string(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 63596e1..b61c73c 100644 >>> --- a/qapi/qapi-util.c >>> +++ b/qapi/qapi-util.c >>> @@ -15,6 +15,7 @@ >>> #include "qapi/error.h" >>> #include "qemu/ctype.h" >>> #include "qapi/qmp/qerror.h" >>> +#include "qapi/qapi-builtin-types.h" >>> >>> CompatPolicy compat_policy; >>> >>> @@ -152,3 +153,25 @@ int parse_qapi_name(const char *str, bool complete) >>> } >>> return p - str; >>> } >>> + >>> +strList *strList_from_string(const char *in, char delim) >>> +{ >>> + strList *res = NULL; >>> + strList **tail = &res; >>> + >>> + while (in && in[0]) { >>> + char *next = strchr(in, delim); >>> + char *value; >>> + >>> + if (next) { >>> + value = g_strndup(in, next - in); >>> + in = next + 1; /* skip the delim */ >>> + } else { >>> + value = g_strdup(in); >>> + in = NULL; >>> + } >>> + QAPI_LIST_APPEND(tail, value); >>> + } >>> + >>> + return res; >>> +} >>> diff --git a/stats/stats-hmp-cmds.c b/stats/stats-hmp-cmds.c >>> index 531e35d..4a2adf8 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 = strList_from_string(names, ','); >>> } >>> QAPI_LIST_PREPEND(request_list, request); >>> } >>> -- >>> 1.8.3.1 >>> >>> >> >>
Alex Bennée <alex.bennee@linaro.org> writes: > Steven Sistare <steven.sistare@oracle.com> writes: > >> On 2/8/2023 1:43 AM, Marc-André Lureau wrote: >>> Hi >>> >>> On Tue, Feb 7, 2023 at 10:50 PM Steve Sistare <steven.sistare@oracle.com> wrote: >>>> >>>> Generalize hmp_split_at_comma() to take any delimiter character, rename >>>> as strList_from_string(), and move it to qapi/util.c. >>>> >>>> No functional change. >>> >>> The g_strsplit() version was a bit simpler, but if you want to >>> optimize it a bit for 1 char delimiter only, ok. >>> >>> Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com> >> >> Yes, and it saves a malloc+free for the array. Small stuff, but I >> thought it worth a few lines of code. Thanks for the speedy review! > > But is the HMP path that performance critical? Otherwise I'd favour > consistent use of the glib APIs because its one less thing to get wrong. The patch reverts my recent commit 0d79271b570 "hmp: Rewrite strlist_from_comma_list() as hmp_split_at_comma()", with a different function name and place, and an additional parameter. There is no explanation for the revert. An intentional revert without even mentioning it would be uncourteous. I don't think this is the case here. I figure you wrote this patch before you saw my commit, then rebased, keeping the old code. A simple rebase mistake, easy enough to correct.
On 2/9/2023 5:02 AM, Markus Armbruster wrote: > Alex Bennée <alex.bennee@linaro.org> writes: > >> Steven Sistare <steven.sistare@oracle.com> writes: >> >>> On 2/8/2023 1:43 AM, Marc-André Lureau wrote: >>>> Hi >>>> >>>> On Tue, Feb 7, 2023 at 10:50 PM Steve Sistare <steven.sistare@oracle.com> wrote: >>>>> >>>>> Generalize hmp_split_at_comma() to take any delimiter character, rename >>>>> as strList_from_string(), and move it to qapi/util.c. >>>>> >>>>> No functional change. >>>> >>>> The g_strsplit() version was a bit simpler, but if you want to >>>> optimize it a bit for 1 char delimiter only, ok. >>>> >>>> Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com> >>> >>> Yes, and it saves a malloc+free for the array. Small stuff, but I >>> thought it worth a few lines of code. Thanks for the speedy review! >> >> But is the HMP path that performance critical? Otherwise I'd favour >> consistent use of the glib APIs because its one less thing to get wrong. > > The patch reverts my recent commit 0d79271b570 "hmp: Rewrite > strlist_from_comma_list() as hmp_split_at_comma()", with a different > function name and place, and an additional parameter. > > There is no explanation for the revert. > > An intentional revert without even mentioning it would be uncourteous. > I don't think this is the case here. I figure you wrote this patch > before you saw my commit, then rebased, keeping the old code. A simple > rebase mistake, easy enough to correct. Hi Markus, I am sorry, I intended no slight. I will document your commit in this commit message. And in response to Alex' comment, I will use your version as the basis of the new function. 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.sistare@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.sistare@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. - Steve
Steven Sistare <steven.sistare@oracle.com> writes: > On 2/9/2023 5:02 AM, Markus Armbruster wrote: >> Alex Bennée <alex.bennee@linaro.org> writes: >> >>> Steven Sistare <steven.sistare@oracle.com> writes: >>> >>>> On 2/8/2023 1:43 AM, Marc-André Lureau wrote: >>>>> Hi >>>>> >>>>> On Tue, Feb 7, 2023 at 10:50 PM Steve Sistare <steven.sistare@oracle.com> wrote: >>>>>> >>>>>> Generalize hmp_split_at_comma() to take any delimiter character, rename >>>>>> as strList_from_string(), and move it to qapi/util.c. >>>>>> >>>>>> No functional change. >>>>> >>>>> The g_strsplit() version was a bit simpler, but if you want to >>>>> optimize it a bit for 1 char delimiter only, ok. >>>>> >>>>> Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com> >>>> >>>> Yes, and it saves a malloc+free for the array. Small stuff, but I >>>> thought it worth a few lines of code. Thanks for the speedy review! >>> >>> But is the HMP path that performance critical? Otherwise I'd favour >>> consistent use of the glib APIs because its one less thing to get wrong. >> >> The patch reverts my recent commit 0d79271b570 "hmp: Rewrite >> strlist_from_comma_list() as hmp_split_at_comma()", with a different >> function name and place, and an additional parameter. >> >> There is no explanation for the revert. >> >> An intentional revert without even mentioning it would be uncourteous. >> I don't think this is the case here. I figure you wrote this patch >> before you saw my commit, then rebased, keeping the old code. A simple >> rebase mistake, easy enough to correct. > > Hi Markus, I am sorry, I intended no slight. Certainly no offense taken :) > I will document your commit > in this commit message. And in response to Alex' comment, I will use your > version as the basis of the new function. > > 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.sistare@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.sistare@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'm also fine with generalizing the function to better serve new uses.
On 2/9/2023 11:46 AM, Markus Armbruster wrote: > Steven Sistare <steven.sistare@oracle.com> writes: > >> On 2/9/2023 5:02 AM, Markus Armbruster wrote: >>> Alex Bennée <alex.bennee@linaro.org> writes: >>> >>>> Steven Sistare <steven.sistare@oracle.com> writes: >>>> >>>>> On 2/8/2023 1:43 AM, Marc-André Lureau wrote: >>>>>> Hi >>>>>> >>>>>> On Tue, Feb 7, 2023 at 10:50 PM Steve Sistare <steven.sistare@oracle.com> wrote: >>>>>>> >>>>>>> Generalize hmp_split_at_comma() to take any delimiter character, rename >>>>>>> as strList_from_string(), and move it to qapi/util.c. >>>>>>> >>>>>>> No functional change. >>>>>> >>>>>> The g_strsplit() version was a bit simpler, but if you want to >>>>>> optimize it a bit for 1 char delimiter only, ok. >>>>>> >>>>>> Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com> >>>>> >>>>> Yes, and it saves a malloc+free for the array. Small stuff, but I >>>>> thought it worth a few lines of code. Thanks for the speedy review! >>>> >>>> But is the HMP path that performance critical? Otherwise I'd favour >>>> consistent use of the glib APIs because its one less thing to get wrong. >>> >>> The patch reverts my recent commit 0d79271b570 "hmp: Rewrite >>> strlist_from_comma_list() as hmp_split_at_comma()", with a different >>> function name and place, and an additional parameter. >>> >>> There is no explanation for the revert. >>> >>> An intentional revert without even mentioning it would be uncourteous. >>> I don't think this is the case here. I figure you wrote this patch >>> before you saw my commit, then rebased, keeping the old code. A simple >>> rebase mistake, easy enough to correct. >> >> Hi Markus, I am sorry, I intended no slight. > > Certainly no offense taken :) > >> I will document your commit >> in this commit message. And in response to Alex' comment, I will use your >> version as the basis of the new function. >> >> 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.sistare@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.sistare@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. - Steve > > I'm also fine with generalizing the function to better serve new uses. >
Steven Sistare <steven.sistare@oracle.com> writes: > On 2/9/2023 11:46 AM, Markus Armbruster wrote: >> Steven Sistare <steven.sistare@oracle.com> writes: >> >>> On 2/9/2023 5:02 AM, Markus Armbruster wrote: >>>> Alex Bennée <alex.bennee@linaro.org> writes: >>>> >>>>> Steven Sistare <steven.sistare@oracle.com> writes: >>>>> >>>>>> On 2/8/2023 1:43 AM, Marc-André Lureau wrote: >>>>>>> Hi >>>>>>> >>>>>>> On Tue, Feb 7, 2023 at 10:50 PM Steve Sistare <steven.sistare@oracle.com> wrote: >>>>>>>> >>>>>>>> Generalize hmp_split_at_comma() to take any delimiter character, rename >>>>>>>> as strList_from_string(), and move it to qapi/util.c. >>>>>>>> >>>>>>>> No functional change. >>>>>>> >>>>>>> The g_strsplit() version was a bit simpler, but if you want to >>>>>>> optimize it a bit for 1 char delimiter only, ok. >>>>>>> >>>>>>> Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com> >>>>>> >>>>>> Yes, and it saves a malloc+free for the array. Small stuff, but I >>>>>> thought it worth a few lines of code. Thanks for the speedy review! >>>>> >>>>> But is the HMP path that performance critical? Otherwise I'd favour >>>>> consistent use of the glib APIs because its one less thing to get wrong. >>>> >>>> The patch reverts my recent commit 0d79271b570 "hmp: Rewrite >>>> strlist_from_comma_list() as hmp_split_at_comma()", with a different >>>> function name and place, and an additional parameter. >>>> >>>> There is no explanation for the revert. >>>> >>>> An intentional revert without even mentioning it would be uncourteous. >>>> I don't think this is the case here. I figure you wrote this patch >>>> before you saw my commit, then rebased, keeping the old code. A simple >>>> rebase mistake, easy enough to correct. >>> >>> Hi Markus, I am sorry, I intended no slight. >> >> Certainly no offense taken :) >> >>> I will document your commit >>> in this commit message. And in response to Alex' comment, I will use your >>> version as the basis of the new function. >>> >>> 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.sistare@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.sistare@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/. > > - Steve > >> >> I'm also fine with generalizing the function to better serve new uses. >>
On 2/9/2023 1:59 PM, Markus Armbruster wrote: > Steven Sistare <steven.sistare@oracle.com> writes: >> On 2/9/2023 11:46 AM, Markus Armbruster wrote: >>> Steven Sistare <steven.sistare@oracle.com> writes: >>> >>>> On 2/9/2023 5:02 AM, Markus Armbruster wrote: >>>>> Alex Bennée <alex.bennee@linaro.org> writes: >>>>> >>>>>> Steven Sistare <steven.sistare@oracle.com> writes: >>>>>> >>>>>>> On 2/8/2023 1:43 AM, Marc-André Lureau wrote: >>>>>>>> Hi >>>>>>>> >>>>>>>> On Tue, Feb 7, 2023 at 10:50 PM Steve Sistare <steven.sistare@oracle.com> wrote: >>>>>>>>> >>>>>>>>> Generalize hmp_split_at_comma() to take any delimiter character, rename >>>>>>>>> as strList_from_string(), and move it to qapi/util.c. >>>>>>>>> >>>>>>>>> No functional change. >>>>>>>> >>>>>>>> The g_strsplit() version was a bit simpler, but if you want to >>>>>>>> optimize it a bit for 1 char delimiter only, ok. >>>>>>>> >>>>>>>> Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com> >>>>>>> >>>>>>> Yes, and it saves a malloc+free for the array. Small stuff, but I >>>>>>> thought it worth a few lines of code. Thanks for the speedy review! >>>>>> >>>>>> But is the HMP path that performance critical? Otherwise I'd favour >>>>>> consistent use of the glib APIs because its one less thing to get wrong. >>>>> >>>>> The patch reverts my recent commit 0d79271b570 "hmp: Rewrite >>>>> strlist_from_comma_list() as hmp_split_at_comma()", with a different >>>>> function name and place, and an additional parameter. >>>>> >>>>> There is no explanation for the revert. >>>>> >>>>> An intentional revert without even mentioning it would be uncourteous. >>>>> I don't think this is the case here. I figure you wrote this patch >>>>> before you saw my commit, then rebased, keeping the old code. A simple >>>>> rebase mistake, easy enough to correct. >>>> >>>> Hi Markus, I am sorry, I intended no slight. >>> >>> Certainly no offense taken :) >>> >>>> I will document your commit >>>> in this commit message. And in response to Alex' comment, I will use your >>>> version as the basis of the new function. >>>> >>>> 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.sistare@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.sistare@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. 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? - Steve
Steven Sistare <steven.sistare@oracle.com> writes: > On 2/9/2023 1:59 PM, Markus Armbruster wrote: >> Steven Sistare <steven.sistare@oracle.com> writes: >>> On 2/9/2023 11:46 AM, Markus Armbruster wrote: >>>> Steven Sistare <steven.sistare@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.sistare@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.sistare@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. > 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. 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?
On 2/10/2023 4:25 AM, Markus Armbruster wrote: > Steven Sistare <steven.sistare@oracle.com> writes: > >> On 2/9/2023 1:59 PM, Markus Armbruster wrote: >>> Steven Sistare <steven.sistare@oracle.com> writes: >>>> On 2/9/2023 11:46 AM, Markus Armbruster wrote: >>>>> Steven Sistare <steven.sistare@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.sistare@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.sistare@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? >
Steven Sistare <steven.sistare@oracle.com> writes: > On 2/10/2023 4:25 AM, Markus Armbruster wrote: >> Steven Sistare <steven.sistare@oracle.com> writes: >> >>> On 2/9/2023 1:59 PM, Markus Armbruster wrote: >>>> Steven Sistare <steven.sistare@oracle.com> writes: >>>>> On 2/9/2023 11:46 AM, Markus Armbruster wrote: >>>>>> Steven Sistare <steven.sistare@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.sistare@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.sistare@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? The design principle is "use JSON to encode structured data as text in QAPI/QMP". Do: "mumble": [1, 2, 3] Don't: "mumble": "1,2,3" We violate the principle in a couple of places. Some are arguably mistakes, some are pragmatic exceptions. The principle implies "the only parser QAPI needs is the JSON parser". By adding other parsers to QAPI, we send a misleading signal, namely that encoding structured data in a way that requires parsing is okay. It's not, generally. So, I'd prefer to find another home for code that splits strings at comma / delimiter. > (and apologies for my long delay in continuing this conversation). I'm in no position to take offense there ;) > - 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?
On 6/13/2023 8:33 AM, Markus Armbruster wrote: > Steven Sistare <steven.sistare@oracle.com> writes: >> On 2/10/2023 4:25 AM, Markus Armbruster wrote: >>> Steven Sistare <steven.sistare@oracle.com> writes: >>>> On 2/9/2023 1:59 PM, Markus Armbruster wrote: >>>>> Steven Sistare <steven.sistare@oracle.com> writes: >>>>>> On 2/9/2023 11:46 AM, Markus Armbruster wrote: >>>>>>> Steven Sistare <steven.sistare@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.sistare@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.sistare@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? > > The design principle is "use JSON to encode structured data as text in > QAPI/QMP". > > Do: "mumble": [1, 2, 3] > > Don't: "mumble": "1,2,3" I don't mumble, but I sometimes mutter and ramble. > We violate the principle in a couple of places. Some are arguably > mistakes, some are pragmatic exceptions. > > The principle implies "the only parser QAPI needs is the JSON parser". > > By adding other parsers to QAPI, we send a misleading signal, namely > that encoding structured data in a way that requires parsing is okay. > It's not, generally. > > So, I'd prefer to find another home for code that splits strings at > comma / delimiter. > >> (and apologies for my long delay in continuing this conversation). > > I'm in no position to take offense there ;) Thanks, that makes it clear. I propose to move strList_from_string and strv_from_strList to new files util/strList.c and include/qemu/strList.h, and leave QAPI_LIST_LENGTH in include/qapi/util.h. (cutil.c already has string functions, but only uses simple C types, so not the best place to add the strList type). Sound OK? - Steve
Steven Sistare <steven.sistare@oracle.com> writes: > On 6/13/2023 8:33 AM, Markus Armbruster wrote: >> Steven Sistare <steven.sistare@oracle.com> writes: [...] >>> What design principle does strList_from_string contradict? Are you OK with >>> putting the simplified version shown above in qapi-util? >> >> The design principle is "use JSON to encode structured data as text in >> QAPI/QMP". >> >> Do: "mumble": [1, 2, 3] >> >> Don't: "mumble": "1,2,3" > > I don't mumble, but I sometimes mutter and ramble. Hah! >> We violate the principle in a couple of places. Some are arguably >> mistakes, some are pragmatic exceptions. >> >> The principle implies "the only parser QAPI needs is the JSON parser". >> >> By adding other parsers to QAPI, we send a misleading signal, namely >> that encoding structured data in a way that requires parsing is okay. >> It's not, generally. >> >> So, I'd prefer to find another home for code that splits strings at >> comma / delimiter. >> >>> (and apologies for my long delay in continuing this conversation). >> >> I'm in no position to take offense there ;) > > Thanks, that makes it clear. > > I propose to move strList_from_string and strv_from_strList to new files > util/strList.c and include/qemu/strList.h, and leave QAPI_LIST_LENGTH in > include/qapi/util.h. > > (cutil.c already has string functions, but only uses simple C types, so > not the best place to add the strList type). > > Sound OK? Works for me. Thanks!
diff --git a/include/monitor/hmp.h b/include/monitor/hmp.h index 2220f14..e80848f 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 81a2b13..7d88b09 100644 --- a/include/qapi/util.h +++ b/include/qapi/util.h @@ -22,6 +22,8 @@ typedef struct QEnumLookup { const int size; } QEnumLookup; +struct strList; + const char *qapi_enum_lookup(const QEnumLookup *lookup, int val); int qapi_enum_parse(const QEnumLookup *lookup, const char *buf, int def, Error **errp); @@ -31,6 +33,13 @@ bool qapi_bool_parse(const char *name, const char *value, bool *obj, int parse_qapi_name(const char *name, bool complete); /* + * Produce a strList from the character delimited string @in. + * All strings are g_strdup'd. + * A NULL or empty input string returns NULL. + */ +struct strList *strList_from_string(const char *in, char delim); + +/* * For any GenericList @list, insert @element at the front. * * Note that this macro evaluates @element exactly once, so it is safe diff --git a/monitor/hmp-cmds.c b/monitor/hmp-cmds.c index 34bd8c6..9665e6e 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 41d326b..30422d9 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 = strList_from_string(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 63596e1..b61c73c 100644 --- a/qapi/qapi-util.c +++ b/qapi/qapi-util.c @@ -15,6 +15,7 @@ #include "qapi/error.h" #include "qemu/ctype.h" #include "qapi/qmp/qerror.h" +#include "qapi/qapi-builtin-types.h" CompatPolicy compat_policy; @@ -152,3 +153,25 @@ int parse_qapi_name(const char *str, bool complete) } return p - str; } + +strList *strList_from_string(const char *in, char delim) +{ + strList *res = NULL; + strList **tail = &res; + + while (in && in[0]) { + char *next = strchr(in, delim); + char *value; + + if (next) { + value = g_strndup(in, next - in); + in = next + 1; /* skip the delim */ + } else { + value = g_strdup(in); + in = NULL; + } + QAPI_LIST_APPEND(tail, value); + } + + return res; +} diff --git a/stats/stats-hmp-cmds.c b/stats/stats-hmp-cmds.c index 531e35d..4a2adf8 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 = strList_from_string(names, ','); } QAPI_LIST_PREPEND(request_list, request); }
Generalize hmp_split_at_comma() to take any delimiter character, rename as strList_from_string(), and move it to qapi/util.c. No functional change. Signed-off-by: Steve Sistare <steven.sistare@oracle.com> --- include/monitor/hmp.h | 1 - include/qapi/util.h | 9 +++++++++ monitor/hmp-cmds.c | 19 ------------------- net/net-hmp-cmds.c | 2 +- qapi/qapi-util.c | 23 +++++++++++++++++++++++ stats/stats-hmp-cmds.c | 2 +- 6 files changed, 34 insertions(+), 22 deletions(-)