| Message ID | 20130424163641.3e15bd43@redhat.com |
|---|---|
| State | New |
| Headers | show |
On 04/24/2013 02:36 PM, Luiz Capitulino wrote: > The drive-mirror command was extended in QEMU v1.3.0 with two new introduced in 1.3, extended in 1.4 > optional arguments 'granularity' and 'buf-size'. However, there's > no way for libvirt to query for the existence of the new arguments. > > This commit solves this problem by adding the > query-drive-mirror-capabilities command, which reports the > existence of both arguments. > > Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com> > --- What if we instead have a generic command querying setup, instead of introducing one query per command? I'm typing this without looking at your patch... { 'type': 'CommandCapability', 'data': { 'command': 'str', 'capabilities': [ 'str' ] } } { 'command': 'query-command-capabilities', 'arguments': { '*command', 'string' }, 'returns': [ 'CommandCapability' ] } with a sample usage: -> { "execute": "query-command-capabilities" } <- { [ { "command": "drive-mirror", "capabilities": [ "granularity", "buf-size" ] }, { "command", ... } ] } Or maybe play a bit with QMP unions to make things more strongly typed: { 'enum': 'DriveMirrorCapability', 'data': { 'buf-size', 'granularity' } } { 'union': 'CommandCapability', 'data': { 'drive-mirror': [ 'DriveMirrorCapability' ], ...: [ ... ] } } { 'command': 'query-command-capabilities', 'arguments': { '*command', 'string' }, 'returns': [ 'CommandCapability' ] } with a sample usage: -> { "execute": "query-command-capabilities" } <- { [ { "type": "drive-mirror", "data": [ "granularity", "buf-size" ] }, { "type", ... } ] } And whether a '*command' argument should be optional for filtered output, vs. always unconditionally dumping all information on all commands with capabilities, vs. mandatory (can only get capabilities for one command at a time), all goes back to the larger question of whether query-* commands should allow filtering. > > I'm just mimicking query-migrate-capabilities. I don't know if this > is the best way of doing this because we don't allow drive-mirror > capabilities to be set and they're always on. > > On the other hand, if we take a simpler approach like returning a > single string of supported new arguments, we may regret it later if > we end up having to support capabilities negotiation. > > Having said that, I don't mind doing this one way or the other and > slightly prefer the simpler approach. ...Now I'm looking at your patch. If we don't go with a fully-generic (or even fully-generic but strongly-typed) version, then your proposal of a new query-drive-mirror-capabilities is probably okay. > > blockdev.c | 21 +++++++++++++++++++++ > qapi-schema.json | 40 ++++++++++++++++++++++++++++++++++++++++ > qmp-commands.hx | 7 +++++++ > 3 files changed, 68 insertions(+) > +++ b/qapi-schema.json > @@ -1715,6 +1715,46 @@ > '*speed': 'int' } } > > ## > +# @DriveMirrorCapability > +# > +# Capabilities supported by the driver-mirror command > +# > +# @granularity: supports setting the dirty bitmap's granularity through the > +# 'granularity' argument > +# > +# @buf-size: supports setting the amount of data to be sent from source > +# to target through the 'buf-size' argument > +# > +# Since: 1.5.0 > +## > +{ 'enum': 'DriveMirrorCapability', 'data': [ 'granularity', 'buf-size' ] } Hey, that matches what I mentioned for the strongly-typed generic command :) > + > +## > +# @DriveMirrorCapabilityStatus > +# > +# Status of drive-mirror capabilities > +# > +# @capability: capability enumeration > +# > +# @state: True if supported False otherwise > +# > +# Since: 1.5.0 > +## > +{ 'type': 'DriveMirrorCapabilityStatus', > + 'data': { 'capability': 'DriveMirrorCapability', 'state': 'bool' } } state will always be true unless we allow negotiation to disable the feature; this may be a bit of overkill for now, but at least the command is designed for extensibility. > + > +## > +# @query-drive-mirror-capabilities > +# > +# Returns information about current drive-mirror's capabilities status > +# > +# Returns: @DriveMirrorCapabilityStatus list > +# > +# Since: 1.5.0 > +## > +{ 'command': 'query-drive-mirror-capabilities', 'returns': [ 'DriveMirrorCapabilityStatus' ] } Seems usable, but I still wonder if my more generic approach is worth considering. Also, I'm not sure if we HAVE to rush this into 1.5; libvirt doesn't (yet) expose the buf-size or granularity options on to the end user (although I do have plans to get to that point); and Paolo pointed out that the try-and-fail approach (omit the argument if the user requests the default, and try the argument relying on qemu's error, instead of probing for the capability) may be sufficient even if we don't have this introspection. Yes, libvirt could give better error messages and/or be more efficient without having to do a try-and-fail approach, but this is something where if we _don't_ add the command to 1.5, and instead focus on getting the command right for 1.6, then downstream distros could easily backport the new command into their build of 1.5, as a quality-of-implementation improvement. > diff --git a/qmp-commands.hx b/qmp-commands.hx > index 4d65422..5b4e559 100644 > --- a/qmp-commands.hx > +++ b/qmp-commands.hx > @@ -2715,6 +2715,13 @@ EQMP > }, > > { > + .name = "query-drive-mirror-capabilities", > + .args_type = "", > + .mhandler.cmd_new = qmp_marshal_input_query_drive_mirror_capabilities, > + }, No example usage?
Il 24/04/2013 22:36, Luiz Capitulino ha scritto: > The drive-mirror command was extended in QEMU v1.3.0 with two new > optional arguments 'granularity' and 'buf-size'. However, there's > no way for libvirt to query for the existence of the new arguments. > > This commit solves this problem by adding the > query-drive-mirror-capabilities command, which reports the > existence of both arguments. > > Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com> NACK, libvirt doesn't need it yet. Let's do it properly in 1.6. Paolo
On Wed, 24 Apr 2013 15:05:23 -0600 Eric Blake <eblake@redhat.com> wrote: > On 04/24/2013 02:36 PM, Luiz Capitulino wrote: > > The drive-mirror command was extended in QEMU v1.3.0 with two new > > introduced in 1.3, extended in 1.4 > > > optional arguments 'granularity' and 'buf-size'. However, there's > > no way for libvirt to query for the existence of the new arguments. > > > > This commit solves this problem by adding the > > query-drive-mirror-capabilities command, which reports the > > existence of both arguments. > > > > Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com> > > --- > > What if we instead have a generic command querying setup, instead of > introducing one query per command? I'm typing this without looking at > your patch... > > { 'type': 'CommandCapability', > 'data': { 'command': 'str', > 'capabilities': [ 'str' ] } } > { 'command': 'query-command-capabilities', > 'arguments': { '*command', 'string' }, > 'returns': [ 'CommandCapability' ] } > > with a sample usage: > > -> { "execute": "query-command-capabilities" } > <- { [ { "command": "drive-mirror", > "capabilities": [ "granularity", "buf-size" ] }, > { "command", ... } > ] } > > > Or maybe play a bit with QMP unions to make things more strongly typed: > > { 'enum': 'DriveMirrorCapability', > 'data': { 'buf-size', 'granularity' } } > { 'union': 'CommandCapability', > 'data': { 'drive-mirror': [ 'DriveMirrorCapability' ], > ...: [ ... ] } } > { 'command': 'query-command-capabilities', > 'arguments': { '*command', 'string' }, > 'returns': [ 'CommandCapability' ] } > > with a sample usage: > > -> { "execute": "query-command-capabilities" } > <- { [ { "type": "drive-mirror", > "data": [ "granularity", "buf-size" ] }, > { "type", ... } > ] } > > And whether a '*command' argument should be optional for filtered > output, vs. always unconditionally dumping all information on all > commands with capabilities, vs. mandatory (can only get capabilities for > one command at a time), all goes back to the larger question of whether > query-* commands should allow filtering. Not discussing filtering for now, but your proposal would superseded by full introspection, wouldn't it? Anyways, I also agree it's a good idea to defer this to 1.6 so we can revisit this topic later.
On Wed, 24 Apr 2013 23:24:37 +0200 Paolo Bonzini <pbonzini@redhat.com> wrote: > Il 24/04/2013 22:36, Luiz Capitulino ha scritto: > > The drive-mirror command was extended in QEMU v1.3.0 with two new > > optional arguments 'granularity' and 'buf-size'. However, there's > > no way for libvirt to query for the existence of the new arguments. > > > > This commit solves this problem by adding the > > query-drive-mirror-capabilities command, which reports the > > existence of both arguments. > > > > Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com> > > NACK, libvirt doesn't need it yet. Let's do it properly in 1.6. NACK taken. Did this on Eric's request but now I see we have all agreed on deferring a proper solution to 1.6.
On 04/24/2013 03:29 PM, Luiz Capitulino wrote: >> -> { "execute": "query-command-capabilities" } >> <- { [ { "type": "drive-mirror", >> "data": [ "granularity", "buf-size" ] }, >> { "type", ... } >> ] } >> >> And whether a '*command' argument should be optional for filtered >> output, vs. always unconditionally dumping all information on all >> commands with capabilities, vs. mandatory (can only get capabilities for >> one command at a time), all goes back to the larger question of whether >> query-* commands should allow filtering. > > Not discussing filtering for now, but your proposal would superseded by > full introspection, wouldn't it? Yeah, the two do seem rather similar, in that they are both providing a form of introspection. As I see it, it boils down to WHAT is being introspected: With query-command-capabilities, we are asking about capabilities, which can always be represented as pure enum values (either the capability is present or it is not). For DriveMirrorCapabilities we happened to map two enum values to the name of two optional drive-mirror parameters; but where we could introduce other capabilities that are unrelated to an optional parameter; also, we aren't polluting the output with parameters that don't really reflect capability additions, so even if we don't provide filtering, the output of a query-command-capabilities will be relatively small compared to the number of total QMP commands. With full-blown command introspection, we would want to be asking for a JSON representation more details about each parameter, in a struct that looks something like { 'name':'str', 'type':'<enumOfTypes>', 'optional':'boolean', '*default':'<typesafe way of representing the default of any boolean option>', '*documentation':'str' }. If we don't provide filtering, then this JSON output might be quite large, because it covers all QMP commands, > > Anyways, I also agree it's a good idea to defer this to 1.6 so we can > revisit this topic later. That relieves some pressure :)
On 04/24/2013 03:59 PM, Eric Blake wrote: > With full-blown command introspection, we would want to be asking for a > JSON representation more details about each parameter, in a struct that > looks something like { 'name':'str', 'type':'<enumOfTypes>', > 'optional':'boolean', '*default':'<typesafe way of representing the > default of any boolean option>', '*documentation':'str' }. If we don't s/boolean option/optional parameter/ > provide filtering, then this JSON output might be quite large, because > it covers all QMP commands, > Whatever we do for full-blown introspection will probably end up quite similar to what we do for command-line introspection; for those following along (bikeshedding on the naming is still happening, but the idea of a struct per command-line option is pretty much agreed on): https://lists.gnu.org/archive/html/qemu-devel/2013-04/msg04929.html
On Wed, 24 Apr 2013 15:59:15 -0600 Eric Blake <eblake@redhat.com> wrote: > On 04/24/2013 03:29 PM, Luiz Capitulino wrote: > >> -> { "execute": "query-command-capabilities" } > >> <- { [ { "type": "drive-mirror", > >> "data": [ "granularity", "buf-size" ] }, > >> { "type", ... } > >> ] } > >> > >> And whether a '*command' argument should be optional for filtered > >> output, vs. always unconditionally dumping all information on all > >> commands with capabilities, vs. mandatory (can only get capabilities for > >> one command at a time), all goes back to the larger question of whether > >> query-* commands should allow filtering. > > > > Not discussing filtering for now, but your proposal would superseded by > > full introspection, wouldn't it? > > Yeah, the two do seem rather similar, in that they are both providing a > form of introspection. As I see it, it boils down to WHAT is being > introspected: > > With query-command-capabilities, we are asking about capabilities, which > can always be represented as pure enum values (either the capability is > present or it is not). For DriveMirrorCapabilities we happened to map > two enum values to the name of two optional drive-mirror parameters; but > where we could introduce other capabilities that are unrelated to an > optional parameter; That's a good point, although I wonder if a command could have a new capability that's not mapped to a new argument. IOW, I'd expect most/all new capabilities to always be mapped to new arguments. In any case, I think the way to go is to add full introspection and then add query-command-capabilities afterwards if it turns out to be necessary.
Il 25/04/2013 14:26, Luiz Capitulino ha scritto: > That's a good point, although I wonder if a command could have a new > capability that's not mapped to a new argument. IOW, I'd expect most/all > new capabilities to always be mapped to new arguments. A new enum value would also be a new capability, but it's handled better by enabling introspection of enum values. Paolo
Paolo Bonzini <pbonzini@redhat.com> writes: > Il 25/04/2013 14:26, Luiz Capitulino ha scritto: >> That's a good point, although I wonder if a command could have a new >> capability that's not mapped to a new argument. IOW, I'd expect most/all >> new capabilities to always be mapped to new arguments. > > A new enum value would also be a new capability, but it's handled better > by enabling introspection of enum values. An extension that adds neither arguments nor argument values is probably an incompatible change, not a proper extension. Don't do that then.
On Fri, 26 Apr 2013 15:40:25 +0200 Markus Armbruster <armbru@redhat.com> wrote: > Paolo Bonzini <pbonzini@redhat.com> writes: > > > Il 25/04/2013 14:26, Luiz Capitulino ha scritto: > >> That's a good point, although I wonder if a command could have a new > >> capability that's not mapped to a new argument. IOW, I'd expect most/all > >> new capabilities to always be mapped to new arguments. > > > > A new enum value would also be a new capability, but it's handled better > > by enabling introspection of enum values. > > An extension that adds neither arguments nor argument values is probably > an incompatible change, not a proper extension. Don't do that then. I think Paolo is referring to a command that takes an enumeration as an argument, eg. transaction.
Luiz Capitulino <lcapitulino@redhat.com> writes: > On Fri, 26 Apr 2013 15:40:25 +0200 > Markus Armbruster <armbru@redhat.com> wrote: > >> Paolo Bonzini <pbonzini@redhat.com> writes: >> >> > Il 25/04/2013 14:26, Luiz Capitulino ha scritto: >> >> That's a good point, although I wonder if a command could have a new >> >> capability that's not mapped to a new argument. IOW, I'd expect most/all >> >> new capabilities to always be mapped to new arguments. >> > >> > A new enum value would also be a new capability, but it's handled better >> > by enabling introspection of enum values. >> >> An extension that adds neither arguments nor argument values is probably >> an incompatible change, not a proper extension. Don't do that then. > > I think Paolo is referring to a command that takes an enumeration as > an argument, eg. transaction. Extending the enumeration adds argument values, i.e. it's just fine.
diff --git a/blockdev.c b/blockdev.c index 8a1652b..3fa5ade 100644 --- a/blockdev.c +++ b/blockdev.c @@ -1279,6 +1279,27 @@ void qmp_block_commit(const char *device, drive_get_ref(drive_get_by_blockdev(bs)); } +DriveMirrorCapabilityStatusList *qmp_query_drive_mirror_capabilities(Error **errp) +{ + DriveMirrorCapabilityStatusList *caps, *head = NULL; + int i; + + for (i = 0; i < DRIVE_MIRROR_CAPABILITY_MAX; i++) { + if (head == NULL) { + head = g_malloc0(sizeof(*caps)); + caps = head; + } else { + caps->next = g_malloc0(sizeof(*caps)); + caps = caps->next; + } + caps->value = g_malloc(sizeof(*caps->value)); + caps->value->capability = i; + caps->value->state = true; + } + + return head; +} + #define DEFAULT_MIRROR_BUF_SIZE (10 << 20) void qmp_drive_mirror(const char *device, const char *target, diff --git a/qapi-schema.json b/qapi-schema.json index 751d3c2..311882d 100644 --- a/qapi-schema.json +++ b/qapi-schema.json @@ -1715,6 +1715,46 @@ '*speed': 'int' } } ## +# @DriveMirrorCapability +# +# Capabilities supported by the driver-mirror command +# +# @granularity: supports setting the dirty bitmap's granularity through the +# 'granularity' argument +# +# @buf-size: supports setting the amount of data to be sent from source +# to target through the 'buf-size' argument +# +# Since: 1.5.0 +## +{ 'enum': 'DriveMirrorCapability', 'data': [ 'granularity', 'buf-size' ] } + +## +# @DriveMirrorCapabilityStatus +# +# Status of drive-mirror capabilities +# +# @capability: capability enumeration +# +# @state: True if supported False otherwise +# +# Since: 1.5.0 +## +{ 'type': 'DriveMirrorCapabilityStatus', + 'data': { 'capability': 'DriveMirrorCapability', 'state': 'bool' } } + +## +# @query-drive-mirror-capabilities +# +# Returns information about current drive-mirror's capabilities status +# +# Returns: @DriveMirrorCapabilityStatus list +# +# Since: 1.5.0 +## +{ 'command': 'query-drive-mirror-capabilities', 'returns': [ 'DriveMirrorCapabilityStatus' ] } + +## # @drive-mirror # # Start mirroring a block device's writes to a new destination. diff --git a/qmp-commands.hx b/qmp-commands.hx index 4d65422..5b4e559 100644 --- a/qmp-commands.hx +++ b/qmp-commands.hx @@ -2715,6 +2715,13 @@ EQMP }, { + .name = "query-drive-mirror-capabilities", + .args_type = "", + .mhandler.cmd_new = qmp_marshal_input_query_drive_mirror_capabilities, + }, + + + { .name = "query-cpu-definitions", .args_type = "", .mhandler.cmd_new = qmp_marshal_input_query_cpu_definitions,
The drive-mirror command was extended in QEMU v1.3.0 with two new optional arguments 'granularity' and 'buf-size'. However, there's no way for libvirt to query for the existence of the new arguments. This commit solves this problem by adding the query-drive-mirror-capabilities command, which reports the existence of both arguments. Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com> --- I'm just mimicking query-migrate-capabilities. I don't know if this is the best way of doing this because we don't allow drive-mirror capabilities to be set and they're always on. On the other hand, if we take a simpler approach like returning a single string of supported new arguments, we may regret it later if we end up having to support capabilities negotiation. Having said that, I don't mind doing this one way or the other and slightly prefer the simpler approach. blockdev.c | 21 +++++++++++++++++++++ qapi-schema.json | 40 ++++++++++++++++++++++++++++++++++++++++ qmp-commands.hx | 7 +++++++ 3 files changed, 68 insertions(+)