Re: [PATCH v8 1/3] monitor/hmp: add support for flag argument with value
* Markus Armbruster (arm...@redhat.com) wrote: > Fabian Ebner writes: > > > Am 09.02.22 um 15:12 schrieb Markus Armbruster: > >> Fabian Ebner 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. > -- Dr. David Alan Gilbert / dgilb...@redhat.com / Manchester, UK
Re: [PATCH v8 1/3] monitor/hmp: add support for flag argument with value
Fabian Ebner writes: > Am 09.02.22 um 15:12 schrieb Markus Armbruster: >> Fabian Ebner 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.
Re: [PATCH v8 1/3] monitor/hmp: add support for flag argument with value
Am 09.02.22 um 15:12 schrieb Markus Armbruster: > Fabian Ebner 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. > >
Re: [PATCH v8 1/3] monitor/hmp: add support for flag argument with value
Fabian Ebner writes:
> From: Stefan Reiter
>
> 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
> Reviewed-by: Eric Blake
> Signed-off-by: Stefan Reiter
> [FE: fixed typo pointed out by Eric Blake]
> Signed-off-by: Fabian Ebner
> ---
> 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), );
> +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.
[PATCH v8 1/3] monitor/hmp: add support for flag argument with value
From: Stefan Reiter
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
Reviewed-by: Eric Blake
Signed-off-by: Stefan Reiter
[FE: fixed typo pointed out by Eric Blake]
Signed-off-by: Fabian Ebner
---
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), );
+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')
*
*/
--
2.30.2
