| Message ID | 1385001528-12003-6-git-send-email-imammedo@redhat.com |
|---|---|
| State | New |
| Headers | show |
Igor Mammedov <imammedo@redhat.com> writes: > Signed-off-by: Igor Mammedov <imammedo@redhat.com> > --- > qapi/string-input-visitor.c | 18 ++++++++++++++++++ > 1 files changed, 18 insertions(+), 0 deletions(-) > > diff --git a/qapi/string-input-visitor.c b/qapi/string-input-visitor.c > index 8f1bc41..a152f5d 100644 > --- a/qapi/string-input-visitor.c > +++ b/qapi/string-input-visitor.c > @@ -97,6 +97,23 @@ static void parse_type_number(Visitor *v, double *obj, const char *name, > *obj = val; > } > > +static void parse_type_size(Visitor *v, uint64_t *obj, const char *name, > + Error **errp) > +{ > + StringInputVisitor *siv = DO_UPCAST(StringInputVisitor, visitor, v); > + int64_t val; > + char *endp; > + > + val = strtosz_suffix(siv->string ? siv->string : "", &endp, > + STRTOSZ_DEFSUFFIX_B); > + if (val < 0 || *endp != '\0') { > + error_set(errp, QERR_INVALID_PARAMETER_VALUE, name, > + "a size value representible as a non-negative int64"); > + return; > + } > + *obj = val; > +} > + > static void parse_start_optional(Visitor *v, bool *present, > const char *name, Error **errp) > { > @@ -131,6 +148,7 @@ StringInputVisitor *string_input_visitor_new(const char *str) > v->visitor.type_bool = parse_type_bool; > v->visitor.type_str = parse_type_str; > v->visitor.type_number = parse_type_number; > + v->visitor.type_size = parse_type_size; > v->visitor.start_optional = parse_start_optional; > > v->string = str; Does this put syntax like "value": "128M" in QMP? If yes, NAK. QMP does not want fancy syntax for numbers, only plain numbers.
On Thu, 21 Nov 2013 11:15:06 +0100 Markus Armbruster <armbru@redhat.com> wrote: > Igor Mammedov <imammedo@redhat.com> writes: > > > Signed-off-by: Igor Mammedov <imammedo@redhat.com> > > --- > > qapi/string-input-visitor.c | 18 ++++++++++++++++++ > > 1 files changed, 18 insertions(+), 0 deletions(-) > > > > diff --git a/qapi/string-input-visitor.c b/qapi/string-input-visitor.c > > index 8f1bc41..a152f5d 100644 > > --- a/qapi/string-input-visitor.c > > +++ b/qapi/string-input-visitor.c > > @@ -97,6 +97,23 @@ static void parse_type_number(Visitor *v, double *obj, const char *name, > > *obj = val; > > } > > > > +static void parse_type_size(Visitor *v, uint64_t *obj, const char *name, > > + Error **errp) > > +{ > > + StringInputVisitor *siv = DO_UPCAST(StringInputVisitor, visitor, v); > > + int64_t val; > > + char *endp; > > + > > + val = strtosz_suffix(siv->string ? siv->string : "", &endp, > > + STRTOSZ_DEFSUFFIX_B); > > + if (val < 0 || *endp != '\0') { > > + error_set(errp, QERR_INVALID_PARAMETER_VALUE, name, > > + "a size value representible as a non-negative int64"); > > + return; > > + } > > + *obj = val; > > +} > > + > > static void parse_start_optional(Visitor *v, bool *present, > > const char *name, Error **errp) > > { > > @@ -131,6 +148,7 @@ StringInputVisitor *string_input_visitor_new(const char *str) > > v->visitor.type_bool = parse_type_bool; > > v->visitor.type_str = parse_type_str; > > v->visitor.type_number = parse_type_number; > > + v->visitor.type_size = parse_type_size; > > v->visitor.start_optional = parse_start_optional; > > > > v->string = str; > > Does this put syntax like "value": "128M" in QMP? If yes, NAK. QMP > does not want fancy syntax for numbers, only plain numbers. > I thought QMP uses its own qmp_visitor, so it shouldn't affect general QMP, if I'm not mistaken. but it allows to use unified memdev_add parser for all interfaces (CLI/HMP/QMP) and it's more user friendly to have on CLI/HMP size=1G vs long integer to express it.
On Mon, Nov 25, 2013 at 04:36:42PM +0100, Igor Mammedov wrote: > On Thu, 21 Nov 2013 11:15:06 +0100 > Markus Armbruster <armbru@redhat.com> wrote: > > > Igor Mammedov <imammedo@redhat.com> writes: > > > > > Signed-off-by: Igor Mammedov <imammedo@redhat.com> > > > --- > > > qapi/string-input-visitor.c | 18 ++++++++++++++++++ > > > 1 files changed, 18 insertions(+), 0 deletions(-) > > > > > > diff --git a/qapi/string-input-visitor.c b/qapi/string-input-visitor.c > > > index 8f1bc41..a152f5d 100644 > > > --- a/qapi/string-input-visitor.c > > > +++ b/qapi/string-input-visitor.c > > > @@ -97,6 +97,23 @@ static void parse_type_number(Visitor *v, double *obj, const char *name, > > > *obj = val; > > > } > > > > > > +static void parse_type_size(Visitor *v, uint64_t *obj, const char *name, > > > + Error **errp) > > > +{ > > > + StringInputVisitor *siv = DO_UPCAST(StringInputVisitor, visitor, v); > > > + int64_t val; > > > + char *endp; > > > + > > > + val = strtosz_suffix(siv->string ? siv->string : "", &endp, > > > + STRTOSZ_DEFSUFFIX_B); > > > + if (val < 0 || *endp != '\0') { > > > + error_set(errp, QERR_INVALID_PARAMETER_VALUE, name, > > > + "a size value representible as a non-negative int64"); > > > + return; > > > + } > > > + *obj = val; > > > +} > > > + > > > static void parse_start_optional(Visitor *v, bool *present, > > > const char *name, Error **errp) > > > { > > > @@ -131,6 +148,7 @@ StringInputVisitor *string_input_visitor_new(const char *str) > > > v->visitor.type_bool = parse_type_bool; > > > v->visitor.type_str = parse_type_str; > > > v->visitor.type_number = parse_type_number; > > > + v->visitor.type_size = parse_type_size; > > > v->visitor.start_optional = parse_start_optional; > > > > > > v->string = str; > > > > Does this put syntax like "value": "128M" in QMP? If yes, NAK. QMP > > does not want fancy syntax for numbers, only plain numbers. > > > > I thought QMP uses its own qmp_visitor, so it shouldn't affect general QMP, > if I'm not mistaken. > > but it allows to use unified memdev_add parser for all interfaces (CLI/HMP/QMP) and > it's more user friendly to have on CLI/HMP size=1G vs long integer to express it. Yes please. Firing up a calculator to figure out how much is 1G is not friendly, neither is firing it up to figure out what did management do with QMP. It should be a text based interface not a binary one.
Il 25/11/2013 17:04, Michael S. Tsirkin ha scritto: >>> > > >>> > > Does this put syntax like "value": "128M" in QMP? If yes, NAK. QMP >>> > > does not want fancy syntax for numbers, only plain numbers. >>> > > >> > >> > I thought QMP uses its own qmp_visitor, so it shouldn't affect general QMP, >> > if I'm not mistaken. >> > >> > but it allows to use unified memdev_add parser for all interfaces (CLI/HMP/QMP) and >> > it's more user friendly to have on CLI/HMP size=1G vs long integer to express it. > Yes please. Firing up a calculator to figure out how much is 1G is not > friendly, neither is firing it up to figure out what did management do > with QMP. It should be a text based interface not a binary one. Wait. :) Command-line and *HMP* object_add (and if we add them, qom_get/qom_set) should be a text based interface. Right now it could be implemented using object_property_parse/print which use the string-input/output-visitor. This is the visitor that this patch touches. QMP object_add (like qom_get/qom-set) should not be at all a text based interface. As Igor says, it should use the QDict-based visitor. This is unfortunately a counter-example to the rule that HMP commands should always be implemented in terms of their QMP counterparts. I do not believe this is really a problem. It can be fixed later; for now, I think "perfect is the enemy of good" applies. Paolo
On 11/25/2013 09:32 AM, Paolo Bonzini wrote: >> Yes please. Firing up a calculator to figure out how much is 1G is not >> friendly, neither is firing it up to figure out what did management do >> with QMP. It should be a text based interface not a binary one. Right now, QMP takes an 'int', which does not allow a suffix. Libvirt prefers passing a value in 'bytes', rather than risking confusion on whether a value in G was rounded (up, down? to nearest power of 10 or power of 2?). Unfortunately, yes, that means you need a calculator when parsing QMP logs to see whether the 1073741824 passed by libvirt is the 1G you had in mind. HMP, qtest, and any other decent shell around raw QMP is more than welcome to provide human-usable wrappers that takes "1G" as a string and turns it into the raw int used by the underlying QMP. In fact, I encourage it. > This is unfortunately a counter-example to the rule that HMP commands > should always be implemented in terms of their QMP counterparts. I do > not believe this is really a problem. It can be fixed later; for now, I > think "perfect is the enemy of good" applies. Hey - I just realized that now that we have anonymous unions, we could theoretically extend QMP to allow a union between 'int' and 'string' - if an 'int' is passed, it is in bytes; if a 'string' is passed, then parse it the way HMP would (so the string "1G" would be equivalent to the raw int 1073741824). But I don't know if it will help you (libvirt will still prefer to use raw ints in any QMP log you read off of libvirt interactions).
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1 Il 25/11/2013 17:43, Eric Blake ha scritto: >>> This is unfortunately a counter-example to the rule that HMP >>> commands should always be implemented in terms of their QMP >>> counterparts. I do not believe this is really a problem. It >>> can be fixed later; for now, I think "perfect is the enemy of >>> good" applies. > Hey - I just realized that now that we have anonymous unions, we > could theoretically extend QMP to allow a union between 'int' and > 'string' - if an 'int' is passed, it is in bytes; if a 'string' is > passed, then parse it the way HMP would (so the string "1G" would > be equivalent to the raw int 1073741824). But I don't know if it > will help you (libvirt will still prefer to use raw ints in any > QMP log you read off of libvirt interactions). No, I think this is not the right solution. The parsing should not be handled at the schema level, because there is really no fixed schema (like device_add for example). Right now, we have (almost) always: HMP QMP | | | | hmp_cont (manually written) qmp_marshal_cont (generated) \ / \ / qmp_cont (manually written) where hmp_cont and qmp_marshal_cont both take QDicts, while qmp_cont takes a list of parameters. object_add cannot be described by a schema, so the implementation should take a Visitor instead, and the would have to be written by hand too: HMP QMP | | | | hmp_object_add (manually written) qmp_object_add (manually written) \ / \ / qapi_object_add(const char *type, const char *id, Visitor *v, Error **errp) However, we don't have the infrastructure to do it; hmp_object_add and qmp_object_add cannot yet be simple wrappers around a common function. We can fix that in due time. Paolo -----BEGIN PGP SIGNATURE----- Version: GnuPG v2.0.22 (GNU/Linux) Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/ iQIcBAEBAgAGBQJSk4KFAAoJEBvWZb6bTYbyyJQP/AlDytO9om7XNu/+/7XGNVYj HxiQSpOo3OgOeyLqJLSV2iBxP1F2YPG3IQ1MQE6aWbPdraMtBt0bnpEQDW5GinX6 f91cnpZi3Yf+3v2/rzmtEs1+3ACOjrn1rVBQiGrbbPiFr7Oa49RD5gWo06M2vZin mfNt3hfUHrmS096IzxGF/sjFstDacnigJZYHHHImqWdHv7BS1I1fK4JptU8ESTEV lLxvphcPKUGoTkLuDSy/6+TwLpr87aLjXFH7GrDUPdmGC/IcJYIDLMP3kjX3KdaA sizqN/pATsxDpOMi/cxnD5onZsFi0J1RiIKxXRbdSLMD0udJUuRrcy19AnUvxhAI gcJy32vj1QWff58S21O7ZbMPCHGClbed3j350wsciumObXneQbcd9iCU//L+Kcfh DqAJmEqSn/kC6vny2JIKL2ZPzETxX6BtnRYIK1lQpYHhy/WC8x9pEs4wltWlVyEQ sY6qq1brNKzCfBpe2WQiHHKHkZ3sw59Pt5bT0ToIpPODIqAyCoeFWh2yXdIEIKmp U7VfBsjk7D6mSqCONgz74Qkjl8olNjlX4vCCXvjTiXghghF84dohNJG7klie75ad arAVmRUTNeR39AY7BCnEKBGFt/OBQal++2ZN/r4AJk7xODs35qF6FMKioAHstt0Z buoUSQmj9X7iuQV3Jriv =fuqh -----END PGP SIGNATURE-----
Eric Blake <eblake@redhat.com> writes: > On 11/25/2013 09:32 AM, Paolo Bonzini wrote: > >>> Yes please. Firing up a calculator to figure out how much is 1G is not >>> friendly, neither is firing it up to figure out what did management do >>> with QMP. It should be a text based interface not a binary one. > > Right now, QMP takes an 'int', which does not allow a suffix. Libvirt > prefers passing a value in 'bytes', rather than risking confusion on > whether a value in G was rounded (up, down? to nearest power of 10 or > power of 2?). Unfortunately, yes, that means you need a calculator when > parsing QMP logs to see whether the 1073741824 passed by libvirt is the > 1G you had in mind. This is by design. > HMP, qtest, and any other decent shell around raw QMP is more than > welcome to provide human-usable wrappers that takes "1G" as a string and > turns it into the raw int used by the underlying QMP. In fact, I > encourage it. Me too, as long as the units suffixes are used consistently. >> This is unfortunately a counter-example to the rule that HMP commands >> should always be implemented in terms of their QMP counterparts. I do >> not believe this is really a problem. It can be fixed later; for now, I >> think "perfect is the enemy of good" applies. I don't get why there's a counter-example. The rules are 1. A QMP command handler (which needs to be of type int (*)(Monitor *, const QDict *params, QObject **ret_data)) should be a thin wrapper around a function with a type appropriate for C that contains all the logic. The wrapper merely translates. 2. HMP commands are to be built on top of these functions, not their handler wrappers. > Hey - I just realized that now that we have anonymous unions, we could > theoretically extend QMP to allow a union between 'int' and 'string' - > if an 'int' is passed, it is in bytes; if a 'string' is passed, then > parse it the way HMP would (so the string "1G" would be equivalent to > the raw int 1073741824). But I don't know if it will help you (libvirt > will still prefer to use raw ints in any QMP log you read off of libvirt > interactions). Please do not complicate QMP's wire format just to help humans deal with it. It's for machines. We intentionally picked a machine-readable syntax that is still readable and writable for humans (feature!), but that's as far as we should go in supporting human use.
Il 27/11/2013 15:15, Markus Armbruster ha scritto: >>> >> This is unfortunately a counter-example to the rule that HMP commands >>> >> should always be implemented in terms of their QMP counterparts. I do >>> >> not believe this is really a problem. It can be fixed later; for now, I >>> >> think "perfect is the enemy of good" applies. > I don't get why there's a counter-example. Take as an example the current implementation of netdev_add. void hmp_netdev_add(Monitor *mon, const QDict *qdict) { ... QemuOpts *opts = qemu_opts_from_qdict(qemu_find_opts("netdev"), qdict, &err); netdev_add(opts, &err); ... } int qmp_netdev_add(Monitor *mon, const QDict *qdict, QObject **ret) { opts_list = qemu_find_opts_err("netdev", &local_err); opts = qemu_opts_from_qdict(opts_list, qdict, &local_err); netdev_add(opts, &local_err); ... return 0; exit_err: ... } These obey the rules you have below (there's just the weirdness that the QMP command handler is not autogenerated). But as you see above, this is really the same code for both handlers, they only differ in error handling minutiae. This is possible with netdev_add (and you could do the same for device_add) because the QMP interface sucks and it is not well defined unless you make all values strings (qemu_opts_from_dict tries to do something meaningful for integers and floats, but given the existence of qdev property types such as hex32 I wouldn't trust it too much). If we want a really different interface between HMP and QMP, one human-oriented and the other JSON-oriented, I'm not sure that you can share much code between the two implementation. Paolo > The rules are > > 1. A QMP command handler (which needs to be of type int (*)(Monitor *, > const QDict *params, QObject **ret_data)) should be a thin wrapper > around a function with a type appropriate for C that contains all the > logic. The wrapper merely translates. > > 2. HMP commands are to be built on top of these functions, not their > handler wrappers.
Paolo Bonzini <pbonzini@redhat.com> writes: > Il 27/11/2013 15:15, Markus Armbruster ha scritto: >>>> >> This is unfortunately a counter-example to the rule that HMP commands >>>> >> should always be implemented in terms of their QMP counterparts. I do >>>> >> not believe this is really a problem. It can be fixed later; for now, I >>>> >> think "perfect is the enemy of good" applies. >> I don't get why there's a counter-example. > > Take as an example the current implementation of netdev_add. > > void hmp_netdev_add(Monitor *mon, const QDict *qdict) > { > ... > QemuOpts *opts = qemu_opts_from_qdict(qemu_find_opts("netdev"), > qdict, &err); > netdev_add(opts, &err); > ... > } > > int qmp_netdev_add(Monitor *mon, const QDict *qdict, QObject **ret) > { > opts_list = qemu_find_opts_err("netdev", &local_err); > opts = qemu_opts_from_qdict(opts_list, qdict, &local_err); > netdev_add(opts, &local_err); > ... > return 0; > > exit_err: > ... > } > > These obey the rules you have below (there's just the weirdness that the QMP > command handler is not autogenerated). > > But as you see above, this is really the same code for both handlers, > they only differ in error handling minutiae. This is possible with > netdev_add (and you could do the same for device_add) because the QMP > interface sucks and it is not well defined unless you make all values > strings (qemu_opts_from_dict tries to do something meaningful for integers > and floats, but given the existence of qdev property types such as hex32 > I wouldn't trust it too much). netdev_add is a problematic example, because its a horrible, horrbible QMP command. I may say that, because I committed the crime :) hmp_netdev_add() is the HMP command handler, and qmp_netdev_add() is the QMP command handler. Their error handling differs only because the former still implements the legacy handler interface. HMP's netdev_add parses arguments into a QemuOpts, exactly like -netdev. I have to admit I can't tell offhand what the heck QMP's netdev_add accepts, because I don't understand what "'*props:': '**'" means in qapi-schema.json. Even today, we permit code to serve as documentation and specification for new features. I wish we didn't. Anyway, whatever it parses ends up in a QDict, as usual. The part that sucks is the use of QemuOpts as netdev_add() parameter, It made some sense when it was done, because then the command line was the only user, its data type for option parameters was (and is) QemuOpts, and QemuOpts was the least inconvenient way to do a function that wants a a discriminated union of parameters, like netdev_add does. It stopped making sense when we started using it from QMP, whose data type for parameters is QDict. I shoehorned netdev_add into QMP in its early days. Hinsight 20/20... In my opinion, use of QemuOpts for anything but parsing parameter strings has become a mistake. Laszlo converted net.c to saner internal interfaces (commit d195325..1a0c095). Perhaps we can build on that. > If we want a really different interface between HMP and QMP, one > human-oriented and the other JSON-oriented, I'm not sure that you can > share much code between the two implementation. You should be able to share everything but the QMP marshalling, and the HMP parsing and printing. > Paolo > >> The rules are >> >> 1. A QMP command handler (which needs to be of type int (*)(Monitor *, >> const QDict *params, QObject **ret_data)) should be a thin wrapper >> around a function with a type appropriate for C that contains all the >> logic. The wrapper merely translates. >> >> 2. HMP commands are to be built on top of these functions, not their >> handler wrappers.
Il 27/11/2013 18:02, Markus Armbruster ha scritto: > I have to admit I can't tell offhand what the heck QMP's netdev_add > accepts, because I don't understand what "'*props:': '**'" means in > qapi-schema.json. Even today, we permit code to serve as documentation > and specification for new features. I wish we didn't. It is ignored, because qmp_netdev_add does not use an automatically-generated unmarshaler. > Anyway, whatever it parses ends up in a QDict, as usual. The QDict comes straight from the monitor dispatch. It is the QDict that ordinarily would be unmarshaled by automatically-generated code. > The part that sucks is the use of QemuOpts as netdev_add() parameter, > > It made some sense when it was done, because then the command line was > the only user, its data type for option parameters was (and is) > QemuOpts, and QemuOpts was the least inconvenient way to do a function > that wants a a discriminated union of parameters, like netdev_add does. > > It stopped making sense when we started using it from QMP, whose data > type for parameters is QDict. I shoehorned netdev_add into QMP in its > early days. Hinsight 20/20... Yes. > In my opinion, use of QemuOpts for anything but parsing parameter > strings has become a mistake. Yes. OTOH QemuOpts is the only reason why QMP and HMP can use a common back-end (netdev_add). With object-add you don't use QemuOpts (for good reasons: you want to separate the human-oriented parsing and the JSON-oriented parsing) and thus you need separate code for QMP and HMP---at least for now. The HMP code can use QemuOpts just like vl.c. The QMP code would use qdict_iter or qdict_first/next and call object_set_property for each element of the dictionary. > Laszlo converted net.c to saner internal interfaces (commit > d195325..1a0c095). Perhaps we can build on that. Yeah, that could be an idea. Another is to convert netdevs to QOM (reusing Laszlo's data structures of course) and then just use object-add and -object. Paolo
On Mon, Nov 25, 2013 at 09:43:48AM -0700, Eric Blake wrote: > On 11/25/2013 09:32 AM, Paolo Bonzini wrote: > > >> Yes please. Firing up a calculator to figure out how much is 1G is not > >> friendly, neither is firing it up to figure out what did management do > >> with QMP. It should be a text based interface not a binary one. > > Right now, QMP takes an 'int', which does not allow a suffix. Libvirt > prefers passing a value in 'bytes', rather than risking confusion on > whether a value in G was rounded (up, down? to nearest power of 10 or > power of 2?). Unfortunately, yes, that means you need a calculator when > parsing QMP logs to see whether the 1073741824 passed by libvirt is the > 1G you had in mind. > > HMP, qtest, and any other decent shell around raw QMP is more than > welcome to provide human-usable wrappers that takes "1G" as a string and > turns it into the raw int used by the underlying QMP. In fact, I > encourage it. How will it know 1G is not e.g. an ID? We can invent rules like "IDs should not start with a number", but these rules are better enforced in a single place for consistency, and it's likely too late to enforce that in HMP. > > This is unfortunately a counter-example to the rule that HMP commands > > should always be implemented in terms of their QMP counterparts. I do > > not believe this is really a problem. It can be fixed later; for now, I > > think "perfect is the enemy of good" applies. > > Hey - I just realized that now that we have anonymous unions, we could > theoretically extend QMP to allow a union between 'int' and 'string' - > if an 'int' is passed, it is in bytes; if a 'string' is passed, then > parse it the way HMP would (so the string "1G" would be equivalent to > the raw int 1073741824). But I don't know if it will help you (libvirt > will still prefer to use raw ints in any QMP log you read off of libvirt > interactions). Yes, I think that would address the issue. > Eric Blake eblake redhat com +1-919-301-3266 > Libvirt virtualization library http://libvirt.org >
"Michael S. Tsirkin" <mst@redhat.com> writes: > On Mon, Nov 25, 2013 at 09:43:48AM -0700, Eric Blake wrote: >> On 11/25/2013 09:32 AM, Paolo Bonzini wrote: >> >> >> Yes please. Firing up a calculator to figure out how much is 1G is not >> >> friendly, neither is firing it up to figure out what did management do >> >> with QMP. It should be a text based interface not a binary one. >> >> Right now, QMP takes an 'int', which does not allow a suffix. Libvirt >> prefers passing a value in 'bytes', rather than risking confusion on >> whether a value in G was rounded (up, down? to nearest power of 10 or >> power of 2?). Unfortunately, yes, that means you need a calculator when >> parsing QMP logs to see whether the 1073741824 passed by libvirt is the >> 1G you had in mind. >> >> HMP, qtest, and any other decent shell around raw QMP is more than >> welcome to provide human-usable wrappers that takes "1G" as a string and >> turns it into the raw int used by the underlying QMP. In fact, I >> encourage it. > > How will it know 1G is not e.g. an ID? > > We can invent rules like "IDs should not start with a number", but these > rules are better enforced in a single place for consistency, and it's > likely too late to enforce that in HMP. This isn't how the human monitor works. Its argument parsing is guided by the command's args_type, which declares argument names and type codes. For instance, type code 's' expects a string argument (may be enclosed in quotes), 'i' a 32-bit integer argument, 'o' a size argument (may be followed by a suffix such as 'G'). If the current argument has type code 'o', then 1G is interpreted as the number 2^30. With type code 's', it's the string "1G". As to rules for IDs: IDs are typically defined via QemuOpts, which restricts them to letters, digits, '-', '.', '_', starting with a letter. >> > This is unfortunately a counter-example to the rule that HMP commands >> > should always be implemented in terms of their QMP counterparts. I do >> > not believe this is really a problem. It can be fixed later; for now, I >> > think "perfect is the enemy of good" applies. >> >> Hey - I just realized that now that we have anonymous unions, we could >> theoretically extend QMP to allow a union between 'int' and 'string' - >> if an 'int' is passed, it is in bytes; if a 'string' is passed, then >> parse it the way HMP would (so the string "1G" would be equivalent to >> the raw int 1073741824). But I don't know if it will help you (libvirt >> will still prefer to use raw ints in any QMP log you read off of libvirt >> interactions). > > Yes, I think that would address the issue. I object, because it goes against the design of QMP.
Il 19/12/2013 15:40, Michael S. Tsirkin ha scritto: > On Mon, Nov 25, 2013 at 09:43:48AM -0700, Eric Blake wrote: >> On 11/25/2013 09:32 AM, Paolo Bonzini wrote: >> >>>> Yes please. Firing up a calculator to figure out how much is 1G is not >>>> friendly, neither is firing it up to figure out what did management do >>>> with QMP. It should be a text based interface not a binary one. >> >> Right now, QMP takes an 'int', which does not allow a suffix. Libvirt >> prefers passing a value in 'bytes', rather than risking confusion on >> whether a value in G was rounded (up, down? to nearest power of 10 or >> power of 2?). Unfortunately, yes, that means you need a calculator when >> parsing QMP logs to see whether the 1073741824 passed by libvirt is the >> 1G you had in mind. >> >> HMP, qtest, and any other decent shell around raw QMP is more than >> welcome to provide human-usable wrappers that takes "1G" as a string and >> turns it into the raw int used by the underlying QMP. In fact, I >> encourage it. > > How will it know 1G is not e.g. an ID? Because all properties are associated to a name. In the case of a memory device, the name could be "size" or "id". When requesting the id, QEMU will use visit_type_str. When requesting the size, QEMU will use visit_type_size. Different functions are called, and the different functions accept different input. To explain further what Markus dismissed as "against the design of QMP", there is another way to choose the way your input is parsed, and that is simply by choosing a different visitor. QMP uses strongly-typed JSON, so it uses a QmpInputVisitor. HMP and command-line just parse text, so it uses an OptsVisitor. So we have: -object memory-ram,size=1M,id=foo (command-line) object_add memory-ram,size=1M,id=foo (HMP) that use OptsVisitor and converts 1M to 1048576. For QMP instead {'command': 'object-add', 'arguments': { 'qom-type': 'memory-ram', 'id': 'foo', 'props': {'size':1048576}}} uses QmpInputVisitor which rejects any 'size' that is not a JSON integer. We definitely do not want to allow QMP input such as this: {'command': 'object-add', 'arguments': { 'qom-type': 'memory-ram', 'id': 'foo', 'props': {'size':'1M'}}} That's against the idea of making QMP as strongly-typed as possible. Paolo > We can invent rules like "IDs should not start with a number", but these > rules are better enforced in a single place for consistency, and it's > likely too late to enforce that in HMP. > >>> This is unfortunately a counter-example to the rule that HMP commands >>> should always be implemented in terms of their QMP counterparts. I do >>> not believe this is really a problem. It can be fixed later; for now, I >>> think "perfect is the enemy of good" applies. >> >> Hey - I just realized that now that we have anonymous unions, we could >> theoretically extend QMP to allow a union between 'int' and 'string' - >> if an 'int' is passed, it is in bytes; if a 'string' is passed, then >> parse it the way HMP would (so the string "1G" would be equivalent to >> the raw int 1073741824). But I don't know if it will help you (libvirt >> will still prefer to use raw ints in any QMP log you read off of libvirt >> interactions). > > Yes, I think that would address the issue. > >> Eric Blake eblake redhat com +1-919-301-3266 >> Libvirt virtualization library http://libvirt.org >> > >
On Thu, Dec 19, 2013 at 04:14:56PM +0100, Markus Armbruster wrote: > "Michael S. Tsirkin" <mst@redhat.com> writes: > > > On Mon, Nov 25, 2013 at 09:43:48AM -0700, Eric Blake wrote: > >> On 11/25/2013 09:32 AM, Paolo Bonzini wrote: > >> > >> >> Yes please. Firing up a calculator to figure out how much is 1G is not > >> >> friendly, neither is firing it up to figure out what did management do > >> >> with QMP. It should be a text based interface not a binary one. > >> > >> Right now, QMP takes an 'int', which does not allow a suffix. Libvirt > >> prefers passing a value in 'bytes', rather than risking confusion on > >> whether a value in G was rounded (up, down? to nearest power of 10 or > >> power of 2?). Unfortunately, yes, that means you need a calculator when > >> parsing QMP logs to see whether the 1073741824 passed by libvirt is the > >> 1G you had in mind. > >> > >> HMP, qtest, and any other decent shell around raw QMP is more than > >> welcome to provide human-usable wrappers that takes "1G" as a string and > >> turns it into the raw int used by the underlying QMP. In fact, I > >> encourage it. > > > > How will it know 1G is not e.g. an ID? > > > > We can invent rules like "IDs should not start with a number", but these > > rules are better enforced in a single place for consistency, and it's > > likely too late to enforce that in HMP. > > This isn't how the human monitor works. Yes but we were talking about a friendly shell for qemu using QMP as a backend. > Its argument parsing is guided by the command's args_type, which > declares argument names and type codes. For instance, type code 's' > expects a string argument (may be enclosed in quotes), 'i' a 32-bit > integer argument, 'o' a size argument (may be followed by a suffix such > as 'G'). > > If the current argument has type code 'o', then 1G is interpreted as the > number 2^30. With type code 's', it's the string "1G". > > As to rules for IDs: IDs are typically defined via QemuOpts, which > restricts them to letters, digits, '-', '.', '_', starting with a > letter. > > >> > This is unfortunately a counter-example to the rule that HMP commands > >> > should always be implemented in terms of their QMP counterparts. I do > >> > not believe this is really a problem. It can be fixed later; for now, I > >> > think "perfect is the enemy of good" applies. > >> > >> Hey - I just realized that now that we have anonymous unions, we could > >> theoretically extend QMP to allow a union between 'int' and 'string' - > >> if an 'int' is passed, it is in bytes; if a 'string' is passed, then > >> parse it the way HMP would (so the string "1G" would be equivalent to > >> the raw int 1073741824). But I don't know if it will help you (libvirt > >> will still prefer to use raw ints in any QMP log you read off of libvirt > >> interactions). > > > > Yes, I think that would address the issue. > > I object, because it goes against the design of QMP.
On Thu, Dec 19, 2013 at 04:31:41PM +0100, Paolo Bonzini wrote: > Il 19/12/2013 15:40, Michael S. Tsirkin ha scritto: > > On Mon, Nov 25, 2013 at 09:43:48AM -0700, Eric Blake wrote: > >> On 11/25/2013 09:32 AM, Paolo Bonzini wrote: > >> > >>>> Yes please. Firing up a calculator to figure out how much is 1G is not > >>>> friendly, neither is firing it up to figure out what did management do > >>>> with QMP. It should be a text based interface not a binary one. > >> > >> Right now, QMP takes an 'int', which does not allow a suffix. Libvirt > >> prefers passing a value in 'bytes', rather than risking confusion on > >> whether a value in G was rounded (up, down? to nearest power of 10 or > >> power of 2?). Unfortunately, yes, that means you need a calculator when > >> parsing QMP logs to see whether the 1073741824 passed by libvirt is the > >> 1G you had in mind. > >> > >> HMP, qtest, and any other decent shell around raw QMP is more than > >> welcome to provide human-usable wrappers that takes "1G" as a string and > >> turns it into the raw int used by the underlying QMP. In fact, I > >> encourage it. > > > > How will it know 1G is not e.g. an ID? > > Because all properties are associated to a name. In the case of a > memory device, the name could be "size" or "id". When requesting the > id, QEMU will use visit_type_str. When requesting the size, QEMU will > use visit_type_size. Different functions are called, and the different > functions accept different input. QEMU can do this, but if you want a frontend with QMP as backend, it won't. So you end up re-implementing this logic in all frontends: instead of just passing on properties they need to know where 1G is a number and where it's a name. > To explain further what Markus dismissed as "against the design of QMP", > there is another way to choose the way your input is parsed, and that is > simply by choosing a different visitor. > > QMP uses strongly-typed JSON, so it uses a QmpInputVisitor. HMP and > command-line just parse text, so it uses an OptsVisitor. > > So we have: > > -object memory-ram,size=1M,id=foo (command-line) > object_add memory-ram,size=1M,id=foo (HMP) > > that use OptsVisitor and converts 1M to 1048576. For QMP instead > > {'command': 'object-add', 'arguments': { > 'qom-type': 'memory-ram', 'id': 'foo', > 'props': {'size':1048576}}} > > uses QmpInputVisitor which rejects any 'size' that is not a JSON > integer. We definitely do not want to allow QMP input such as this: > > {'command': 'object-add', 'arguments': { > 'qom-type': 'memory-ram', 'id': 'foo', > 'props': {'size':'1M'}}} > > That's against the idea of making QMP as strongly-typed as possible. > > Paolo > > > We can invent rules like "IDs should not start with a number", but these > > rules are better enforced in a single place for consistency, and it's > > likely too late to enforce that in HMP. > > > >>> This is unfortunately a counter-example to the rule that HMP commands > >>> should always be implemented in terms of their QMP counterparts. I do > >>> not believe this is really a problem. It can be fixed later; for now, I > >>> think "perfect is the enemy of good" applies. > >> > >> Hey - I just realized that now that we have anonymous unions, we could > >> theoretically extend QMP to allow a union between 'int' and 'string' - > >> if an 'int' is passed, it is in bytes; if a 'string' is passed, then > >> parse it the way HMP would (so the string "1G" would be equivalent to > >> the raw int 1073741824). But I don't know if it will help you (libvirt > >> will still prefer to use raw ints in any QMP log you read off of libvirt > >> interactions). > > > > Yes, I think that would address the issue. > > > >> Eric Blake eblake redhat com +1-919-301-3266 > >> Libvirt virtualization library http://libvirt.org > >> > > > > >
Il 19/12/2013 16:40, Michael S. Tsirkin ha scritto: > On Thu, Dec 19, 2013 at 04:31:41PM +0100, Paolo Bonzini wrote: >> Il 19/12/2013 15:40, Michael S. Tsirkin ha scritto: >>> On Mon, Nov 25, 2013 at 09:43:48AM -0700, Eric Blake wrote: >>>> On 11/25/2013 09:32 AM, Paolo Bonzini wrote: >>>> >>>>>> Yes please. Firing up a calculator to figure out how much is 1G is not >>>>>> friendly, neither is firing it up to figure out what did management do >>>>>> with QMP. It should be a text based interface not a binary one. >>>> >>>> Right now, QMP takes an 'int', which does not allow a suffix. Libvirt >>>> prefers passing a value in 'bytes', rather than risking confusion on >>>> whether a value in G was rounded (up, down? to nearest power of 10 or >>>> power of 2?). Unfortunately, yes, that means you need a calculator when >>>> parsing QMP logs to see whether the 1073741824 passed by libvirt is the >>>> 1G you had in mind. >>>> >>>> HMP, qtest, and any other decent shell around raw QMP is more than >>>> welcome to provide human-usable wrappers that takes "1G" as a string and >>>> turns it into the raw int used by the underlying QMP. In fact, I >>>> encourage it. >>> >>> How will it know 1G is not e.g. an ID? >> >> Because all properties are associated to a name. In the case of a >> memory device, the name could be "size" or "id". When requesting the >> id, QEMU will use visit_type_str. When requesting the size, QEMU will >> use visit_type_size. Different functions are called, and the different >> functions accept different input. > > QEMU can do this, but if you want a frontend with QMP as backend, > it won't. > So you end up re-implementing this logic in all frontends: > instead of just passing on properties they need to know > where 1G is a number and where it's a name. You have schema introspection that lets you know if a property is an int or a size. OptsVisitor chose to accept 1G, but this absolutely need not be universal. Human interfaces must not be designed according to QEMU's own whims. Another front-end may want to differentiate 1GB from 1GiB, a graphical front-end may want to present different widgets; they can do it because they see "size" in the schema but in any case the "final" communication to QEMU via QMP will be an int. Paolo
diff --git a/qapi/string-input-visitor.c b/qapi/string-input-visitor.c index 8f1bc41..a152f5d 100644 --- a/qapi/string-input-visitor.c +++ b/qapi/string-input-visitor.c @@ -97,6 +97,23 @@ static void parse_type_number(Visitor *v, double *obj, const char *name, *obj = val; } +static void parse_type_size(Visitor *v, uint64_t *obj, const char *name, + Error **errp) +{ + StringInputVisitor *siv = DO_UPCAST(StringInputVisitor, visitor, v); + int64_t val; + char *endp; + + val = strtosz_suffix(siv->string ? siv->string : "", &endp, + STRTOSZ_DEFSUFFIX_B); + if (val < 0 || *endp != '\0') { + error_set(errp, QERR_INVALID_PARAMETER_VALUE, name, + "a size value representible as a non-negative int64"); + return; + } + *obj = val; +} + static void parse_start_optional(Visitor *v, bool *present, const char *name, Error **errp) { @@ -131,6 +148,7 @@ StringInputVisitor *string_input_visitor_new(const char *str) v->visitor.type_bool = parse_type_bool; v->visitor.type_str = parse_type_str; v->visitor.type_number = parse_type_number; + v->visitor.type_size = parse_type_size; v->visitor.start_optional = parse_start_optional; v->string = str;
Signed-off-by: Igor Mammedov <imammedo@redhat.com> --- qapi/string-input-visitor.c | 18 ++++++++++++++++++ 1 files changed, 18 insertions(+), 0 deletions(-)