| Message ID | 20220204101220.343526-2-f.ebner@proxmox.com |
|---|---|
| State | New |
| Headers | show |
| Series | VNC-related HMP/QMP fixes | expand |
Fabian Ebner <f.ebner@proxmox.com> writes: > From: Stefan Reiter <s.reiter@proxmox.com> > > Adds support for the "-xV" parameter type, where "-x" denotes a flag > name and the "V" suffix indicates that this flag is supposed to take > an arbitrary string parameter. > > These parameters are always optional, the entry in the qdict will be > omitted if the flag is not given. > > Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com> > Reviewed-by: Eric Blake <eblake@redhat.com> > Signed-off-by: Stefan Reiter <s.reiter@proxmox.com> > [FE: fixed typo pointed out by Eric Blake] > Signed-off-by: Fabian Ebner <f.ebner@proxmox.com> > --- > monitor/hmp.c | 19 ++++++++++++++++++- > monitor/monitor-internal.h | 3 ++- > 2 files changed, 20 insertions(+), 2 deletions(-) > > diff --git a/monitor/hmp.c b/monitor/hmp.c > index b20737e63c..fd4f5866c7 100644 > --- a/monitor/hmp.c > +++ b/monitor/hmp.c > @@ -981,6 +981,7 @@ static QDict *monitor_parse_arguments(Monitor *mon, > { > const char *tmp = p; > int skip_key = 0; > + int ret; > /* option */ > > c = *typestr++; > @@ -1003,11 +1004,27 @@ static QDict *monitor_parse_arguments(Monitor *mon, > } > if (skip_key) { > p = tmp; > + } else if (*typestr == 'V') { > + /* has option with string value */ > + typestr++; > + tmp = p++; > + while (qemu_isspace(*p)) { > + p++; > + } > + ret = get_str(buf, sizeof(buf), &p); > + if (ret < 0) { > + monitor_printf(mon, "%s: value expected for -%c\n", > + cmd->name, *tmp); > + goto fail; > + } > + qdict_put_str(qdict, key, buf); > } else { > - /* has option */ > + /* has boolean option */ > p++; > qdict_put_bool(qdict, key, true); > } > + } else if (*typestr == 'V') { > + typestr++; > } > } > break; > diff --git a/monitor/monitor-internal.h b/monitor/monitor-internal.h > index 3da3f86c6a..a4cb307c8a 100644 > --- a/monitor/monitor-internal.h > +++ b/monitor/monitor-internal.h > @@ -63,7 +63,8 @@ > * '.' other form of optional type (for 'i' and 'l') > * 'b' boolean > * user mode accepts "on" or "off" > - * '-' optional parameter (eg. '-f') > + * '-' optional parameter (eg. '-f'); if followed by a 'V', it > + * specifies an optional string param (e.g. '-fV' allows '-f foo') > * > */ For what it's worth, getopt() uses ':' after the option character for "takes an argument". Happy to make that tweak in my tree. But see my review of PATCH 3 first.
Am 09.02.22 um 15:12 schrieb Markus Armbruster: > Fabian Ebner <f.ebner@proxmox.com> writes: ----8<---- >> diff --git a/monitor/monitor-internal.h b/monitor/monitor-internal.h >> index 3da3f86c6a..a4cb307c8a 100644 >> --- a/monitor/monitor-internal.h >> +++ b/monitor/monitor-internal.h >> @@ -63,7 +63,8 @@ >> * '.' other form of optional type (for 'i' and 'l') >> * 'b' boolean >> * user mode accepts "on" or "off" >> - * '-' optional parameter (eg. '-f') >> + * '-' optional parameter (eg. '-f'); if followed by a 'V', it >> + * specifies an optional string param (e.g. '-fV' allows '-f foo') >> * >> */ > > For what it's worth, getopt() uses ':' after the option character for > "takes an argument". > Doing that leads to e.g. .args_type = "protocol:s,password:s,display:-d:,connected:s?", so there's two different kinds of colons now. It's not a problem functionality-wise AFAICT, but it might not be ideal. Should I still go for it? Also, wouldn't future non-string flag parameters need their own letter too? What about re-using 's' here instead? > Happy to make that tweak in my tree. But see my review of PATCH 3 > first. > >
Fabian Ebner <f.ebner@proxmox.com> writes: > Am 09.02.22 um 15:12 schrieb Markus Armbruster: >> Fabian Ebner <f.ebner@proxmox.com> writes: > > ----8<---- > >>> diff --git a/monitor/monitor-internal.h b/monitor/monitor-internal.h >>> index 3da3f86c6a..a4cb307c8a 100644 >>> --- a/monitor/monitor-internal.h >>> +++ b/monitor/monitor-internal.h >>> @@ -63,7 +63,8 @@ >>> * '.' other form of optional type (for 'i' and 'l') >>> * 'b' boolean >>> * user mode accepts "on" or "off" >>> - * '-' optional parameter (eg. '-f') >>> + * '-' optional parameter (eg. '-f'); if followed by a 'V', it >>> + * specifies an optional string param (e.g. '-fV' allows '-f foo') >>> * >>> */ >> >> For what it's worth, getopt() uses ':' after the option character for >> "takes an argument". >> > > Doing that leads to e.g. > .args_type = "protocol:s,password:s,display:-d:,connected:s?", > so there's two different kinds of colons now. Point. > It's not a problem > functionality-wise AFAICT, but it might not be ideal. Should I still go > for it? > > Also, wouldn't future non-string flag parameters need their own letter > too? What about re-using 's' here instead? Another good point. Dave, what do you think? >> Happy to make that tweak in my tree. But see my review of PATCH 3 >> first.
* Markus Armbruster (armbru@redhat.com) wrote: > Fabian Ebner <f.ebner@proxmox.com> writes: > > > Am 09.02.22 um 15:12 schrieb Markus Armbruster: > >> Fabian Ebner <f.ebner@proxmox.com> writes: > > > > ----8<---- > > > >>> diff --git a/monitor/monitor-internal.h b/monitor/monitor-internal.h > >>> index 3da3f86c6a..a4cb307c8a 100644 > >>> --- a/monitor/monitor-internal.h > >>> +++ b/monitor/monitor-internal.h > >>> @@ -63,7 +63,8 @@ > >>> * '.' other form of optional type (for 'i' and 'l') > >>> * 'b' boolean > >>> * user mode accepts "on" or "off" > >>> - * '-' optional parameter (eg. '-f') > >>> + * '-' optional parameter (eg. '-f'); if followed by a 'V', it > >>> + * specifies an optional string param (e.g. '-fV' allows '-f foo') > >>> * > >>> */ > >> > >> For what it's worth, getopt() uses ':' after the option character for > >> "takes an argument". > >> > > > > Doing that leads to e.g. > > .args_type = "protocol:s,password:s,display:-d:,connected:s?", > > so there's two different kinds of colons now. > > Point. Yeh, : is probably a bad idea. > > It's not a problem > > functionality-wise AFAICT, but it might not be ideal. Should I still go > > for it? > > > > Also, wouldn't future non-string flag parameters need their own letter > > too? What about re-using 's' here instead? > > Another good point. > > Dave, what do you think? Yeh, I'd be happy reusing 's' here. Dave > >> Happy to make that tweak in my tree. But see my review of PATCH 3 > >> first. >
diff --git a/monitor/hmp.c b/monitor/hmp.c index b20737e63c..fd4f5866c7 100644 --- a/monitor/hmp.c +++ b/monitor/hmp.c @@ -981,6 +981,7 @@ static QDict *monitor_parse_arguments(Monitor *mon, { const char *tmp = p; int skip_key = 0; + int ret; /* option */ c = *typestr++; @@ -1003,11 +1004,27 @@ static QDict *monitor_parse_arguments(Monitor *mon, } if (skip_key) { p = tmp; + } else if (*typestr == 'V') { + /* has option with string value */ + typestr++; + tmp = p++; + while (qemu_isspace(*p)) { + p++; + } + ret = get_str(buf, sizeof(buf), &p); + if (ret < 0) { + monitor_printf(mon, "%s: value expected for -%c\n", + cmd->name, *tmp); + goto fail; + } + qdict_put_str(qdict, key, buf); } else { - /* has option */ + /* has boolean option */ p++; qdict_put_bool(qdict, key, true); } + } else if (*typestr == 'V') { + typestr++; } } break; diff --git a/monitor/monitor-internal.h b/monitor/monitor-internal.h index 3da3f86c6a..a4cb307c8a 100644 --- a/monitor/monitor-internal.h +++ b/monitor/monitor-internal.h @@ -63,7 +63,8 @@ * '.' other form of optional type (for 'i' and 'l') * 'b' boolean * user mode accepts "on" or "off" - * '-' optional parameter (eg. '-f') + * '-' optional parameter (eg. '-f'); if followed by a 'V', it + * specifies an optional string param (e.g. '-fV' allows '-f foo') * */