| Message ID | 13ae3f12913b517c7f2e5e356b54b6bf872045da.1400123059.git.jcody@redhat.com |
|---|---|
| State | New |
| Headers | show |
The Wednesday 14 May 2014 à 23:20:18 (-0400), Jeff Cody wrote : > This modifies the block operation block-commit so that it will > accept node-name arguments for either 'top' or 'base' BDS. > > The filename and node-name are mutually exclusive to each other; > i.e.: > "top" and "top-node-name" are mutually exclusive (enforced) > "base" and "base-node-name" are mutually exclusive (enforced) > > The preferred and recommended method now of using block-commit > is to specify the BDS to operate on via their node-name arguments. > > This also adds an explicit check that base_bs is in the chain of > top_bs, because if they are referenced by their unique node-name > arguments, the discovery process does not intrinsically verify > they are in the same chain. > > Signed-off-by: Jeff Cody <jcody@redhat.com> > --- > blockdev.c | 35 +++++++++++++++++++++++++++++++++-- > qapi-schema.json | 35 ++++++++++++++++++++++++++--------- > qmp-commands.hx | 29 ++++++++++++++++++++++------- > 3 files changed, 81 insertions(+), 18 deletions(-) > > diff --git a/blockdev.c b/blockdev.c > index 02c6525..d8cb396 100644 > --- a/blockdev.c > +++ b/blockdev.c > @@ -1869,7 +1869,9 @@ void qmp_block_stream(const char *device, bool has_base, > > void qmp_block_commit(const char *device, > bool has_base, const char *base, > + bool has_base_node_name, const char *base_node_name, > bool has_top, const char *top, > + bool has_top_node_name, const char *top_node_name, > bool has_speed, int64_t speed, > Error **errp) > { > @@ -1888,6 +1890,15 @@ void qmp_block_commit(const char *device, > /* drain all i/o before commits */ > bdrv_drain_all(); > > + if (has_base && has_base_node_name) { > + error_setg(errp, "'base' and 'base-node-name' are mutually exclusive"); > + return; > + } > + if (has_top && has_top_node_name) { > + error_setg(errp, "'top' and 'top-node-name' are mutually exclusive"); > + return; > + } > + > bs = bdrv_find(device); > if (!bs) { > error_set(errp, QERR_DEVICE_NOT_FOUND, device); > @@ -1897,7 +1908,14 @@ void qmp_block_commit(const char *device, > /* default top_bs is the active layer */ > top_bs = bs; > > - if (top) { > + /* Find the 'top' image file for the commit */ > + if (has_top_node_name) { > + top_bs = bdrv_lookup_bs(NULL, top_node_name, &local_err); > + if (local_err) { > + error_propagate(errp, local_err); > + return; > + } > + } else if (has_top && top) { > if (strcmp(bs->filename, top) != 0) { > top_bs = bdrv_find_backing_image(bs, top); > } > @@ -1908,7 +1926,14 @@ void qmp_block_commit(const char *device, > return; > } > > - if (has_base && base) { > + /* Find the 'base' image file for the commit */ > + if (has_base_node_name) { > + base_bs = bdrv_lookup_bs(NULL, base_node_name, &local_err); > + if (local_err) { > + error_propagate(errp, local_err); > + return; > + } > + } else if (has_base && base) { > base_bs = bdrv_find_backing_image(top_bs, base); > } else { > base_bs = bdrv_find_base(top_bs); > @@ -1919,6 +1944,12 @@ void qmp_block_commit(const char *device, > return; > } > > + /* Verify that 'base' is in the same chain as 'top' */ > + if (!bdrv_is_in_chain(top_bs, base_bs)) { > + error_setg(errp, "'base' and 'top' are not in the same chain"); > + return; > + } > + > if (top_bs == bs) { > commit_active_start(bs, base_bs, speed, on_error, block_job_cb, > bs, &local_err); > diff --git a/qapi-schema.json b/qapi-schema.json > index 06a9b5d..eddb2b8 100644 > --- a/qapi-schema.json > +++ b/qapi-schema.json > @@ -2096,12 +2096,27 @@ > # > # @device: the name of the device > # > -# @base: #optional The file name of the backing image to write data into. > -# If not specified, this is the deepest backing image > +# For 'base', either @base or @base-node-name may be set but not both. If > +# neither is specified, this is the deepest backing image > # > -# @top: #optional The file name of the backing image within the image chain, > -# which contains the topmost data to be committed down. If > -# not specified, this is the active layer. > +# @base: #optional The file name of the backing image to write data > +# into. > +# > +# @base-node-name #optional The name of the block driver state node of the > +# backing image to write data into. > +# (Since 2.1) > +# > +# For 'top', either @top or @top-node-name must be set but not both. > +# > +# @top: #optional The file name of the backing image within the image > +# chain, which contains the topmost data to be > +# committed down. If not specified, this is the > +# active layer. > +# > +# @top-node-name: #optional The block driver state node name of the backing > +# image within the image chain, which contains the > +# topmost data to be committed down. > +# (Since 2.1) > # > # If top == base, that is an error. > # If top == active, the job will not be completed by itself, > @@ -2120,17 +2135,19 @@ > # > # Returns: Nothing on success > # If commit or stream is already active on this device, DeviceInUse > -# If @device does not exist, DeviceNotFound > +# If @device does not exist or cannot be determined, DeviceNotFound > # If image commit is not supported by this device, NotSupported > -# If @base or @top is invalid, a generic error is returned > +# If @base, @top, @base-node-name, @top-node-name invalid, GenericError > # If @speed is invalid, InvalidParameter > +# If both @base and @base-node-name are specified, GenericError > +# If both @top and @top-node-name are specified, GenericError > # > # Since: 1.3 > # > ## > { 'command': 'block-commit', > - 'data': { 'device': 'str', '*base': 'str', '*top': 'str', > - '*speed': 'int' } } > + 'data': { 'device': 'str', '*base': 'str', '*base-node-name': 'str', > + '*top': 'str', '*top-node-name': 'str', '*speed': 'int' } } > > ## > # @drive-backup > diff --git a/qmp-commands.hx b/qmp-commands.hx > index 1aa3c65..2b9b1dc 100644 > --- a/qmp-commands.hx > +++ b/qmp-commands.hx > @@ -985,7 +985,7 @@ EQMP > > { > .name = "block-commit", > - .args_type = "device:B,base:s?,top:s?,speed:o?", > + .args_type = "device:B,base:s?,base-node-name:s?,top:s?,top-node-name:s?,speed:o?", > .mhandler.cmd_new = qmp_marshal_input_block_commit, > }, > > @@ -999,12 +999,27 @@ data between 'top' and 'base' into 'base'. > Arguments: > > - "device": The device's ID, must be unique (json-string) > -- "base": The file name of the backing image to write data into. > - If not specified, this is the deepest backing image > - (json-string, optional) > -- "top": The file name of the backing image within the image chain, > - which contains the topmost data to be committed down. If > - not specified, this is the active layer. (json-string, optional) > + > +For 'base', either base or base-node-name may be set but not both. If > +neither is specified, this is the deepest backing image > + > +- "base": The file name of the backing image to write data into. > + (json-string, optional) > +- "base-node-name": The name of the block driver state node of the > + backing image to write data into. > + (json-string, optional) (Since 2.1) > + > +For 'top', either top or top-node-name must be set but not both. > + > +- "top": The file name of the backing image within the image chain, > + which contains the topmost data to be committed down. If > + not specified, this is the active layer. > + (json-string, optional) > + > +- "top-node-name": The block driver state node name of the backing > + image within the image chain, which contains the > + topmost data to be committed down. > + (json-string, optional) (Since 2.1) ^ (cosmetic) this block is not aligned with the upper ones. > > If top == base, that is an error. > If top == active, the job will not be completed by itself, > -- > 1.8.3.1 >
On 05/14/2014 09:20 PM, Jeff Cody wrote: > This modifies the block operation block-commit so that it will > accept node-name arguments for either 'top' or 'base' BDS. > > The filename and node-name are mutually exclusive to each other; > i.e.: > "top" and "top-node-name" are mutually exclusive (enforced) > "base" and "base-node-name" are mutually exclusive (enforced) Hmm - we have a bug in existing commands for NOT forcing mutual exclusion even though the schema says they are exclusive. For example, qmp_block_passwd() blindly calls: bs = bdrv_lookup_bs(has_device ? device : NULL, has_node_name ? node_name : NULL, &local_err); and _that_ function blindly favors device name over node name, rather than erroring out if both are supplied. I think we should fix that first - I'd rather that bdrv_lookup_bs either errors out if both names are supplied (rather than making each caller repeat the work), or that it checks that if both names are supplied then it resolves to the same bs. Hmm - if I have device "foo" with chain "base <- top", would bdrv_lookup_bs("foo", "base") be okay ("base" is a node-name that resides within "device"'s chain) or an error ("base" resolves to a different bs than "device")? Again, all the more reason to have a common function decide the semantics we want, then all other clients automatically pick up on those semantics. > > The preferred and recommended method now of using block-commit > is to specify the BDS to operate on via their node-name arguments. > > This also adds an explicit check that base_bs is in the chain of > top_bs, because if they are referenced by their unique node-name > arguments, the discovery process does not intrinsically verify > they are in the same chain. I like this addition. > > Signed-off-by: Jeff Cody <jcody@redhat.com> > --- > blockdev.c | 35 +++++++++++++++++++++++++++++++++-- > qapi-schema.json | 35 ++++++++++++++++++++++++++--------- > qmp-commands.hx | 29 ++++++++++++++++++++++------- > 3 files changed, 81 insertions(+), 18 deletions(-) > > > - if (top) { > + /* Find the 'top' image file for the commit */ > + if (has_top_node_name) { > + top_bs = bdrv_lookup_bs(NULL, top_node_name, &local_err); Hmm. Shouldn't you ALSO verify that 'top_bs' belongs to the chain owned by 'device'? Later on, you verify that 'base_bs' belongs to 'top_bs', but if I pass node names, those names could have found a top unrelated to 'device'; and base related to that top. Maybe that gives more weight to my idea of possibly allowing bdrv_lookup_bs(device, top_node_name), where the lookup succeeds even when device and top_node_name resolve to different bs, but where top_node_name is a node in the chain of device. > > - if (has_base && base) { > + /* Find the 'base' image file for the commit */ > + if (has_base_node_name) { > + base_bs = bdrv_lookup_bs(NULL, base_node_name, &local_err); > + if (local_err) { > + error_propagate(errp, local_err); > + return; > + } > + } else if (has_base && base) { Here, it doesn't matter whether you pass device or NULL for the device name... > > + /* Verify that 'base' is in the same chain as 'top' */ > + if (!bdrv_is_in_chain(top_bs, base_bs)) { > + error_setg(errp, "'base' and 'top' are not in the same chain"); > + return; ...because this check is still mandatory to prove that a user with chain: base <- sn1 <- sn2 <- active is not calling 'device':'active', 'top-node-name':'sn1', 'base-node-name':'sn2' (all that bdrv_lookup_bs can do is verify that base-node-name belongs to device, not that it _also_ belongs to top_bs). > -# @base: #optional The file name of the backing image to write data into. > -# If not specified, this is the deepest backing image > +# For 'base', either @base or @base-node-name may be set but not both. If > +# neither is specified, this is the deepest backing image I like how you factored th is out up front... > # > -# @top: #optional The file name of the backing image within the image chain, > -# which contains the topmost data to be committed down. If > -# not specified, this is the active layer. > +# @base: #optional The file name of the backing image to write data > +# into. > +# > +# @base-node-name #optional The name of the block driver state node of the > +# backing image to write data into. > +# (Since 2.1) > +# > +# For 'top', either @top or @top-node-name must be set but not both. ...but here, you were not consistent. I'd add "If neither is specified, this is the active image" here... > +# > +# @top: #optional The file name of the backing image within the image > +# chain, which contains the topmost data to be > +# committed down. If not specified, this is the > +# active layer. ...and drop the second sentence from here. > +# > +# @top-node-name: #optional The block driver state node name of the backing > +# image within the image chain, which contains the > +# topmost data to be committed down. > +# (Since 2.1) > # > # If top == base, that is an error. > # If top == active, the job will not be completed by itself, > @@ -2120,17 +2135,19 @@ > # > # Returns: Nothing on success > # If commit or stream is already active on this device, DeviceInUse > -# If @device does not exist, DeviceNotFound > +# If @device does not exist or cannot be determined, DeviceNotFound > # If image commit is not supported by this device, NotSupported > -# If @base or @top is invalid, a generic error is returned > +# If @base, @top, @base-node-name, @top-node-name invalid, GenericError > # If @speed is invalid, InvalidParameter > +# If both @base and @base-node-name are specified, GenericError > +# If both @top and @top-node-name are specified, GenericError These last two are arguably sub-conditions of the earlier statement about @top and @top-node-name being invalid (since being invalid can include when both strings are used at once). It wouldn't hurt my feelings to reduce the docs by two lines. > # Since: 1.3 > # > ## > { 'command': 'block-commit', > - 'data': { 'device': 'str', '*base': 'str', '*top': 'str', > - '*speed': 'int' } } > + 'data': { 'device': 'str', '*base': 'str', '*base-node-name': 'str', > + '*top': 'str', '*top-node-name': 'str', '*speed': 'int' } } I'm okay with keeping 'device' mandatory, even though technically the use of a node-name could imply which device is intended. That is, as long as the code correctly errors out when device and top-node-name don't resolve to the same chain :) > + > +For 'top', either top or top-node-name must be set but not both. > + > +- "top": The file name of the backing image within the image chain, > + which contains the topmost data to be committed down. If > + not specified, this is the active layer. > + (json-string, optional) Same blurb about hoisting the 'not specified => active' sentence to the common text, rather than leaving it in the 'top' text.
On Thu, May 15, 2014 at 09:42:07AM -0600, Eric Blake wrote: > On 05/14/2014 09:20 PM, Jeff Cody wrote: > > This modifies the block operation block-commit so that it will > > accept node-name arguments for either 'top' or 'base' BDS. > > > > The filename and node-name are mutually exclusive to each other; > > i.e.: > > "top" and "top-node-name" are mutually exclusive (enforced) > > "base" and "base-node-name" are mutually exclusive (enforced) > > Hmm - we have a bug in existing commands for NOT forcing mutual > exclusion even though the schema says they are exclusive. Those are a slightly different scenario, however. In this patch, we are dealing with a filename string, and a node-name string. There is more of a 1:1 relationship between those two (both identify a particular image). The other existing commands, however, deal with a device name string (identifying a drive chain) or a node-name string (identifying a particular image). That said, I think the other commands should be modified to enforce the documentation, but it doesn't really relate to this patch. > For example, > qmp_block_passwd() blindly calls: > > bs = bdrv_lookup_bs(has_device ? device : NULL, > has_node_name ? node_name : NULL, > &local_err); > > and _that_ function blindly favors device name over node name, rather > than erroring out if both are supplied. I find this an odd function to begin with, because node_name uniquely identifies any given BDS from any chain. Device name will uniquely identify a whole chain, by returning its top-most BDS. The function seems confused. > I think we should fix that > first - I'd rather that bdrv_lookup_bs either errors out if both names > are supplied (rather than making each caller repeat the work), or that > it checks that if both names are supplied then it resolves to the same bs. > I disagree on this part, at least partially. It think it needs to be changed, but I don't think it needs to be part of this series. I think that can be a separate series, to address that across the other QAPI commands that accept node-name and device name > Hmm - if I have device "foo" with chain "base <- top", would > bdrv_lookup_bs("foo", "base") be okay ("base" is a node-name that > resides within "device"'s chain) or an error ("base" resolves to a > different bs than "device")? Again, all the more reason to have a > common function decide the semantics we want, then all other clients > automatically pick up on those semantics. > > > > > The preferred and recommended method now of using block-commit > > is to specify the BDS to operate on via their node-name arguments. > > > > This also adds an explicit check that base_bs is in the chain of > > top_bs, because if they are referenced by their unique node-name > > arguments, the discovery process does not intrinsically verify > > they are in the same chain. > > I like this addition. > > > > > Signed-off-by: Jeff Cody <jcody@redhat.com> > > --- > > blockdev.c | 35 +++++++++++++++++++++++++++++++++-- > > qapi-schema.json | 35 ++++++++++++++++++++++++++--------- > > qmp-commands.hx | 29 ++++++++++++++++++++++------- > > 3 files changed, 81 insertions(+), 18 deletions(-) > > > > > > > > > - if (top) { > > + /* Find the 'top' image file for the commit */ > > + if (has_top_node_name) { > > + top_bs = bdrv_lookup_bs(NULL, top_node_name, &local_err); > > Hmm. Shouldn't you ALSO verify that 'top_bs' belongs to the chain owned > by 'device'? Later on, you verify that 'base_bs' belongs to 'top_bs', > but if I pass node names, those names could have found a top unrelated > to 'device'; and base related to that top. > It is not needed - it is not relevant for the active commit case (the current check will catch that, since top_bs == bs). And for the non-active commit case, commit_start() will return an error if 'top_bs' is not in 'bs', so the case of 'base_bs' or 'top_bs' not being in 'device' is already covered. > Maybe that gives more weight to my idea of possibly allowing > bdrv_lookup_bs(device, top_node_name), where the lookup succeeds even > when device and top_node_name resolve to different bs, but where > top_node_name is a node in the chain of device. > > > > > - if (has_base && base) { > > + /* Find the 'base' image file for the commit */ > > + if (has_base_node_name) { > > + base_bs = bdrv_lookup_bs(NULL, base_node_name, &local_err); > > + if (local_err) { > > + error_propagate(errp, local_err); > > + return; > > + } > > + } else if (has_base && base) { > > Here, it doesn't matter whether you pass device or NULL for the device > name... > This conditional is just to determine if we are finding the backing image referenced by the string "base" or just using the default bottom-most base image. > > > > + /* Verify that 'base' is in the same chain as 'top' */ > > + if (!bdrv_is_in_chain(top_bs, base_bs)) { > > + error_setg(errp, "'base' and 'top' are not in the same chain"); > > + return; > > ...because this check is still mandatory to prove that a user with chain: > base <- sn1 <- sn2 <- active > > is not calling 'device':'active', 'top-node-name':'sn1', > 'base-node-name':'sn2' (all that bdrv_lookup_bs can do is verify that > base-node-name belongs to device, not that it _also_ belongs to top_bs). > > > > -# @base: #optional The file name of the backing image to write data into. > > -# If not specified, this is the deepest backing image > > +# For 'base', either @base or @base-node-name may be set but not both. If > > +# neither is specified, this is the deepest backing image > > I like how you factored th is out up front... > > > # > > -# @top: #optional The file name of the backing image within the image chain, > > -# which contains the topmost data to be committed down. If > > -# not specified, this is the active layer. > > +# @base: #optional The file name of the backing image to write data > > +# into. > > +# > > +# @base-node-name #optional The name of the block driver state node of the > > +# backing image to write data into. > > +# (Since 2.1) > > +# > > +# For 'top', either @top or @top-node-name must be set but not both. > > ...but here, you were not consistent. I'd add "If neither is specified, > this is the active image" here... > OK, thanks. > > +# > > +# @top: #optional The file name of the backing image within the image > > +# chain, which contains the topmost data to be > > +# committed down. If not specified, this is the > > +# active layer. > > ...and drop the second sentence from here. > OK, thanks. > > +# > > +# @top-node-name: #optional The block driver state node name of the backing > > +# image within the image chain, which contains the > > +# topmost data to be committed down. > > +# (Since 2.1) > > # > > # If top == base, that is an error. > > # If top == active, the job will not be completed by itself, > > @@ -2120,17 +2135,19 @@ > > # > > # Returns: Nothing on success > > # If commit or stream is already active on this device, DeviceInUse > > -# If @device does not exist, DeviceNotFound > > +# If @device does not exist or cannot be determined, DeviceNotFound > > # If image commit is not supported by this device, NotSupported > > -# If @base or @top is invalid, a generic error is returned > > +# If @base, @top, @base-node-name, @top-node-name invalid, GenericError > > # If @speed is invalid, InvalidParameter > > +# If both @base and @base-node-name are specified, GenericError > > +# If both @top and @top-node-name are specified, GenericError > > These last two are arguably sub-conditions of the earlier statement > about @top and @top-node-name being invalid (since being invalid can > include when both strings are used at once). It wouldn't hurt my > feelings to reduce the docs by two lines. > OK. I called it out explicitly, since it is currently different behavior from the other functions. > > # Since: 1.3 > > # > > ## > > { 'command': 'block-commit', > > - 'data': { 'device': 'str', '*base': 'str', '*top': 'str', > > - '*speed': 'int' } } > > + 'data': { 'device': 'str', '*base': 'str', '*base-node-name': 'str', > > + '*top': 'str', '*top-node-name': 'str', '*speed': 'int' } } > > I'm okay with keeping 'device' mandatory, even though technically the > use of a node-name could imply which device is intended. That is, as > long as the code correctly errors out when device and top-node-name > don't resolve to the same chain :) > I had thought about making it optional, but I do think it is better as mandatory - it makes sure the user knows which chain they are operating on, so it acts as a (small) check against user mistakes. > > > + > > +For 'top', either top or top-node-name must be set but not both. > > + > > +- "top": The file name of the backing image within the image chain, > > + which contains the topmost data to be committed down. If > > + not specified, this is the active layer. > > + (json-string, optional) > > Same blurb about hoisting the 'not specified => active' sentence to the > common text, rather than leaving it in the 'top' text. > OK, thanks.
diff --git a/blockdev.c b/blockdev.c index 02c6525..d8cb396 100644 --- a/blockdev.c +++ b/blockdev.c @@ -1869,7 +1869,9 @@ void qmp_block_stream(const char *device, bool has_base, void qmp_block_commit(const char *device, bool has_base, const char *base, + bool has_base_node_name, const char *base_node_name, bool has_top, const char *top, + bool has_top_node_name, const char *top_node_name, bool has_speed, int64_t speed, Error **errp) { @@ -1888,6 +1890,15 @@ void qmp_block_commit(const char *device, /* drain all i/o before commits */ bdrv_drain_all(); + if (has_base && has_base_node_name) { + error_setg(errp, "'base' and 'base-node-name' are mutually exclusive"); + return; + } + if (has_top && has_top_node_name) { + error_setg(errp, "'top' and 'top-node-name' are mutually exclusive"); + return; + } + bs = bdrv_find(device); if (!bs) { error_set(errp, QERR_DEVICE_NOT_FOUND, device); @@ -1897,7 +1908,14 @@ void qmp_block_commit(const char *device, /* default top_bs is the active layer */ top_bs = bs; - if (top) { + /* Find the 'top' image file for the commit */ + if (has_top_node_name) { + top_bs = bdrv_lookup_bs(NULL, top_node_name, &local_err); + if (local_err) { + error_propagate(errp, local_err); + return; + } + } else if (has_top && top) { if (strcmp(bs->filename, top) != 0) { top_bs = bdrv_find_backing_image(bs, top); } @@ -1908,7 +1926,14 @@ void qmp_block_commit(const char *device, return; } - if (has_base && base) { + /* Find the 'base' image file for the commit */ + if (has_base_node_name) { + base_bs = bdrv_lookup_bs(NULL, base_node_name, &local_err); + if (local_err) { + error_propagate(errp, local_err); + return; + } + } else if (has_base && base) { base_bs = bdrv_find_backing_image(top_bs, base); } else { base_bs = bdrv_find_base(top_bs); @@ -1919,6 +1944,12 @@ void qmp_block_commit(const char *device, return; } + /* Verify that 'base' is in the same chain as 'top' */ + if (!bdrv_is_in_chain(top_bs, base_bs)) { + error_setg(errp, "'base' and 'top' are not in the same chain"); + return; + } + if (top_bs == bs) { commit_active_start(bs, base_bs, speed, on_error, block_job_cb, bs, &local_err); diff --git a/qapi-schema.json b/qapi-schema.json index 06a9b5d..eddb2b8 100644 --- a/qapi-schema.json +++ b/qapi-schema.json @@ -2096,12 +2096,27 @@ # # @device: the name of the device # -# @base: #optional The file name of the backing image to write data into. -# If not specified, this is the deepest backing image +# For 'base', either @base or @base-node-name may be set but not both. If +# neither is specified, this is the deepest backing image # -# @top: #optional The file name of the backing image within the image chain, -# which contains the topmost data to be committed down. If -# not specified, this is the active layer. +# @base: #optional The file name of the backing image to write data +# into. +# +# @base-node-name #optional The name of the block driver state node of the +# backing image to write data into. +# (Since 2.1) +# +# For 'top', either @top or @top-node-name must be set but not both. +# +# @top: #optional The file name of the backing image within the image +# chain, which contains the topmost data to be +# committed down. If not specified, this is the +# active layer. +# +# @top-node-name: #optional The block driver state node name of the backing +# image within the image chain, which contains the +# topmost data to be committed down. +# (Since 2.1) # # If top == base, that is an error. # If top == active, the job will not be completed by itself, @@ -2120,17 +2135,19 @@ # # Returns: Nothing on success # If commit or stream is already active on this device, DeviceInUse -# If @device does not exist, DeviceNotFound +# If @device does not exist or cannot be determined, DeviceNotFound # If image commit is not supported by this device, NotSupported -# If @base or @top is invalid, a generic error is returned +# If @base, @top, @base-node-name, @top-node-name invalid, GenericError # If @speed is invalid, InvalidParameter +# If both @base and @base-node-name are specified, GenericError +# If both @top and @top-node-name are specified, GenericError # # Since: 1.3 # ## { 'command': 'block-commit', - 'data': { 'device': 'str', '*base': 'str', '*top': 'str', - '*speed': 'int' } } + 'data': { 'device': 'str', '*base': 'str', '*base-node-name': 'str', + '*top': 'str', '*top-node-name': 'str', '*speed': 'int' } } ## # @drive-backup diff --git a/qmp-commands.hx b/qmp-commands.hx index 1aa3c65..2b9b1dc 100644 --- a/qmp-commands.hx +++ b/qmp-commands.hx @@ -985,7 +985,7 @@ EQMP { .name = "block-commit", - .args_type = "device:B,base:s?,top:s?,speed:o?", + .args_type = "device:B,base:s?,base-node-name:s?,top:s?,top-node-name:s?,speed:o?", .mhandler.cmd_new = qmp_marshal_input_block_commit, }, @@ -999,12 +999,27 @@ data between 'top' and 'base' into 'base'. Arguments: - "device": The device's ID, must be unique (json-string) -- "base": The file name of the backing image to write data into. - If not specified, this is the deepest backing image - (json-string, optional) -- "top": The file name of the backing image within the image chain, - which contains the topmost data to be committed down. If - not specified, this is the active layer. (json-string, optional) + +For 'base', either base or base-node-name may be set but not both. If +neither is specified, this is the deepest backing image + +- "base": The file name of the backing image to write data into. + (json-string, optional) +- "base-node-name": The name of the block driver state node of the + backing image to write data into. + (json-string, optional) (Since 2.1) + +For 'top', either top or top-node-name must be set but not both. + +- "top": The file name of the backing image within the image chain, + which contains the topmost data to be committed down. If + not specified, this is the active layer. + (json-string, optional) + +- "top-node-name": The block driver state node name of the backing + image within the image chain, which contains the + topmost data to be committed down. + (json-string, optional) (Since 2.1) If top == base, that is an error. If top == active, the job will not be completed by itself,
This modifies the block operation block-commit so that it will accept node-name arguments for either 'top' or 'base' BDS. The filename and node-name are mutually exclusive to each other; i.e.: "top" and "top-node-name" are mutually exclusive (enforced) "base" and "base-node-name" are mutually exclusive (enforced) The preferred and recommended method now of using block-commit is to specify the BDS to operate on via their node-name arguments. This also adds an explicit check that base_bs is in the chain of top_bs, because if they are referenced by their unique node-name arguments, the discovery process does not intrinsically verify they are in the same chain. Signed-off-by: Jeff Cody <jcody@redhat.com> --- blockdev.c | 35 +++++++++++++++++++++++++++++++++-- qapi-schema.json | 35 ++++++++++++++++++++++++++--------- qmp-commands.hx | 29 ++++++++++++++++++++++------- 3 files changed, 81 insertions(+), 18 deletions(-)