| Message ID | 53efdb963d57e99fd0f77a34b1d3a860b3f6d2aa.1400123059.git.jcody@redhat.com |
|---|---|
| State | New |
| Headers | show |
The Wednesday 14 May 2014 à 23:20:17 (-0400), Jeff Cody wrote : > Now that active layer block-commit is supported, the 'top' argument > no longer needs to be mandatory. > > Change it optional, with the default being the active layer in the Do you mean "Change it to optional" or "Make it optional" ? > device chain. > > Signed-off-by: Jeff Cody <jcody@redhat.com> > --- > blockdev.c | 3 ++- > qapi-schema.json | 7 ++++--- > qmp-commands.hx | 5 +++-- > tests/qemu-iotests/040 | 28 ++++++++++++++++++---------- > 4 files changed, 27 insertions(+), 16 deletions(-) > > diff --git a/blockdev.c b/blockdev.c > index 500707e..02c6525 100644 > --- a/blockdev.c > +++ b/blockdev.c > @@ -1868,7 +1868,8 @@ void qmp_block_stream(const char *device, bool has_base, > } > > void qmp_block_commit(const char *device, > - bool has_base, const char *base, const char *top, > + bool has_base, const char *base, > + bool has_top, const char *top, > bool has_speed, int64_t speed, > Error **errp) > { > diff --git a/qapi-schema.json b/qapi-schema.json > index 36cb964..06a9b5d 100644 > --- a/qapi-schema.json > +++ b/qapi-schema.json > @@ -2099,8 +2099,9 @@ > # @base: #optional The file name of the backing image to write data into. > # If not specified, this is the deepest backing image > # > -# @top: The file name of the backing image within the image chain, > -# which contains the topmost data to be committed down. > +# @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. > # > # If top == base, that is an error. > # If top == active, the job will not be completed by itself, > @@ -2128,7 +2129,7 @@ > # > ## > { 'command': 'block-commit', > - 'data': { 'device': 'str', '*base': 'str', 'top': 'str', > + 'data': { 'device': 'str', '*base': 'str', '*top': 'str', > '*speed': 'int' } } > > ## > diff --git a/qmp-commands.hx b/qmp-commands.hx > index cae890e..1aa3c65 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?,top:s?,speed:o?", > .mhandler.cmd_new = qmp_marshal_input_block_commit, > }, > > @@ -1003,7 +1003,8 @@ Arguments: > 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. > + which contains the topmost data to be committed down. If > + not specified, this is the active layer. (json-string, optional) > > If top == base, that is an error. > If top == active, the job will not be completed by itself, > diff --git a/tests/qemu-iotests/040 b/tests/qemu-iotests/040 > index 734b6a6..803b0c7 100755 > --- a/tests/qemu-iotests/040 > +++ b/tests/qemu-iotests/040 > @@ -35,11 +35,7 @@ test_img = os.path.join(iotests.test_dir, 'test.img') > class ImageCommitTestCase(iotests.QMPTestCase): > '''Abstract base class for image commit test cases''' > > - def run_commit_test(self, top, base): > - self.assert_no_active_block_jobs() > - result = self.vm.qmp('block-commit', device='drive0', top=top, base=base) > - self.assert_qmp(result, 'return', {}) > - > + def wait_for_complete(self): > completed = False > while not completed: > for event in self.vm.get_qmp_events(wait=True): > @@ -58,6 +54,18 @@ class ImageCommitTestCase(iotests.QMPTestCase): > self.assert_no_active_block_jobs() > self.vm.shutdown() > > + def run_commit_test(self, top, base): > + self.assert_no_active_block_jobs() > + result = self.vm.qmp('block-commit', device='drive0', top=top, base=base) > + self.assert_qmp(result, 'return', {}) > + self.wait_for_complete() > + > + def run_default_commit_test(self): > + self.assert_no_active_block_jobs() > + result = self.vm.qmp('block-commit', device='drive0') > + self.assert_qmp(result, 'return', {}) > + self.wait_for_complete() > + > class TestSingleDrive(ImageCommitTestCase): > image_len = 1 * 1024 * 1024 > test_len = 1 * 1024 * 256 > @@ -109,17 +117,17 @@ class TestSingleDrive(ImageCommitTestCase): > self.assertEqual(-1, qemu_io('-c', 'read -P 0xab 0 524288', backing_img).find("verification failed")) > self.assertEqual(-1, qemu_io('-c', 'read -P 0xef 524288 524288', backing_img).find("verification failed")) > > + def test_top_is_default_active(self): > + self.run_default_commit_test() > + self.assertEqual(-1, qemu_io('-c', 'read -P 0xab 0 524288', backing_img).find("verification failed")) > + self.assertEqual(-1, qemu_io('-c', 'read -P 0xef 524288 524288', backing_img).find("verification failed")) > + > def test_top_and_base_reversed(self): > self.assert_no_active_block_jobs() > result = self.vm.qmp('block-commit', device='drive0', top='%s' % backing_img, base='%s' % mid_img) > self.assert_qmp(result, 'error/class', 'GenericError') > self.assert_qmp(result, 'error/desc', 'Base \'%s\' not found' % mid_img) > > - def test_top_omitted(self): > - self.assert_no_active_block_jobs() > - result = self.vm.qmp('block-commit', device='drive0') > - self.assert_qmp(result, 'error/class', 'GenericError') > - self.assert_qmp(result, 'error/desc', "Parameter 'top' is missing") > > class TestRelativePaths(ImageCommitTestCase): > image_len = 1 * 1024 * 1024 > -- > 1.8.3.1 > Reviewed-by: Benoit Canet <benoit@irqsave.net>
On Thu, May 15, 2014 at 01:47:55PM +0200, Benoît Canet wrote: > The Wednesday 14 May 2014 à 23:20:17 (-0400), Jeff Cody wrote : > > Now that active layer block-commit is supported, the 'top' argument > > no longer needs to be mandatory. > > > > Change it optional, with the default being the active layer in the > > Do you mean "Change it to optional" or "Make it optional" ? > Thanks - forgot the "to". > > device chain. > > > > Signed-off-by: Jeff Cody <jcody@redhat.com> > > --- > > blockdev.c | 3 ++- > > qapi-schema.json | 7 ++++--- > > qmp-commands.hx | 5 +++-- > > tests/qemu-iotests/040 | 28 ++++++++++++++++++---------- > > 4 files changed, 27 insertions(+), 16 deletions(-) > > > > diff --git a/blockdev.c b/blockdev.c > > index 500707e..02c6525 100644 > > --- a/blockdev.c > > +++ b/blockdev.c > > @@ -1868,7 +1868,8 @@ void qmp_block_stream(const char *device, bool has_base, > > } > > > > void qmp_block_commit(const char *device, > > - bool has_base, const char *base, const char *top, > > + bool has_base, const char *base, > > + bool has_top, const char *top, > > bool has_speed, int64_t speed, > > Error **errp) > > { > > diff --git a/qapi-schema.json b/qapi-schema.json > > index 36cb964..06a9b5d 100644 > > --- a/qapi-schema.json > > +++ b/qapi-schema.json > > @@ -2099,8 +2099,9 @@ > > # @base: #optional The file name of the backing image to write data into. > > # If not specified, this is the deepest backing image > > # > > -# @top: The file name of the backing image within the image chain, > > -# which contains the topmost data to be committed down. > > +# @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. > > # > > # If top == base, that is an error. > > # If top == active, the job will not be completed by itself, > > @@ -2128,7 +2129,7 @@ > > # > > ## > > { 'command': 'block-commit', > > - 'data': { 'device': 'str', '*base': 'str', 'top': 'str', > > + 'data': { 'device': 'str', '*base': 'str', '*top': 'str', > > '*speed': 'int' } } > > > > ## > > diff --git a/qmp-commands.hx b/qmp-commands.hx > > index cae890e..1aa3c65 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?,top:s?,speed:o?", > > .mhandler.cmd_new = qmp_marshal_input_block_commit, > > }, > > > > @@ -1003,7 +1003,8 @@ Arguments: > > 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. > > + which contains the topmost data to be committed down. If > > + not specified, this is the active layer. (json-string, optional) > > > > If top == base, that is an error. > > If top == active, the job will not be completed by itself, > > diff --git a/tests/qemu-iotests/040 b/tests/qemu-iotests/040 > > index 734b6a6..803b0c7 100755 > > --- a/tests/qemu-iotests/040 > > +++ b/tests/qemu-iotests/040 > > @@ -35,11 +35,7 @@ test_img = os.path.join(iotests.test_dir, 'test.img') > > class ImageCommitTestCase(iotests.QMPTestCase): > > '''Abstract base class for image commit test cases''' > > > > - def run_commit_test(self, top, base): > > - self.assert_no_active_block_jobs() > > - result = self.vm.qmp('block-commit', device='drive0', top=top, base=base) > > - self.assert_qmp(result, 'return', {}) > > - > > + def wait_for_complete(self): > > completed = False > > while not completed: > > for event in self.vm.get_qmp_events(wait=True): > > @@ -58,6 +54,18 @@ class ImageCommitTestCase(iotests.QMPTestCase): > > self.assert_no_active_block_jobs() > > self.vm.shutdown() > > > > + def run_commit_test(self, top, base): > > + self.assert_no_active_block_jobs() > > + result = self.vm.qmp('block-commit', device='drive0', top=top, base=base) > > + self.assert_qmp(result, 'return', {}) > > + self.wait_for_complete() > > + > > + def run_default_commit_test(self): > > + self.assert_no_active_block_jobs() > > + result = self.vm.qmp('block-commit', device='drive0') > > + self.assert_qmp(result, 'return', {}) > > + self.wait_for_complete() > > + > > class TestSingleDrive(ImageCommitTestCase): > > image_len = 1 * 1024 * 1024 > > test_len = 1 * 1024 * 256 > > @@ -109,17 +117,17 @@ class TestSingleDrive(ImageCommitTestCase): > > self.assertEqual(-1, qemu_io('-c', 'read -P 0xab 0 524288', backing_img).find("verification failed")) > > self.assertEqual(-1, qemu_io('-c', 'read -P 0xef 524288 524288', backing_img).find("verification failed")) > > > > + def test_top_is_default_active(self): > > + self.run_default_commit_test() > > + self.assertEqual(-1, qemu_io('-c', 'read -P 0xab 0 524288', backing_img).find("verification failed")) > > + self.assertEqual(-1, qemu_io('-c', 'read -P 0xef 524288 524288', backing_img).find("verification failed")) > > + > > def test_top_and_base_reversed(self): > > self.assert_no_active_block_jobs() > > result = self.vm.qmp('block-commit', device='drive0', top='%s' % backing_img, base='%s' % mid_img) > > self.assert_qmp(result, 'error/class', 'GenericError') > > self.assert_qmp(result, 'error/desc', 'Base \'%s\' not found' % mid_img) > > > > - def test_top_omitted(self): > > - self.assert_no_active_block_jobs() > > - result = self.vm.qmp('block-commit', device='drive0') > > - self.assert_qmp(result, 'error/class', 'GenericError') > > - self.assert_qmp(result, 'error/desc', "Parameter 'top' is missing") > > > > class TestRelativePaths(ImageCommitTestCase): > > image_len = 1 * 1024 * 1024 > > -- > > 1.8.3.1 > > > Reviewed-by: Benoit Canet <benoit@irqsave.net>
On 05/14/2014 09:20 PM, Jeff Cody wrote: > Now that active layer block-commit is supported, the 'top' argument > no longer needs to be mandatory. > > Change it optional, with the default being the active layer in the > device chain. Good, this is an API-compatible change (doesn't break old clients that always provide 'top'). I'm definitely in favor of it. > > Signed-off-by: Jeff Cody <jcody@redhat.com> > --- > blockdev.c | 3 ++- > qapi-schema.json | 7 ++++--- > qmp-commands.hx | 5 +++-- > tests/qemu-iotests/040 | 28 ++++++++++++++++++---------- > 4 files changed, 27 insertions(+), 16 deletions(-) > > +++ b/qapi-schema.json > @@ -2099,8 +2099,9 @@ > # @base: #optional The file name of the backing image to write data into. > # If not specified, this is the deepest backing image > # > -# @top: The file name of the backing image within the image chain, > -# which contains the topmost data to be committed down. > +# @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. We have not done the conversion from mandatory to optional very often; I wonder if it is worth an annotation of when the conversion happened (so someone reading just this version of the schema but planning on targeting older qemu knows to always supply the argument). Something like: @top: since 1.3, #optional since 2.1; The file name... But that feels rather long. It's not a show-stopper to me, particularly since we still don't have any automated conversion from these comments into any other documentation formats. That is, if we decide it is worth adding hints about when something changed from mandatory to optional, it would be its own patch and cover the work we've done on other recent changes like block_passwd. Reviewed-by: Eric Blake <eblake@redhat.com>
diff --git a/blockdev.c b/blockdev.c index 500707e..02c6525 100644 --- a/blockdev.c +++ b/blockdev.c @@ -1868,7 +1868,8 @@ void qmp_block_stream(const char *device, bool has_base, } void qmp_block_commit(const char *device, - bool has_base, const char *base, const char *top, + bool has_base, const char *base, + bool has_top, const char *top, bool has_speed, int64_t speed, Error **errp) { diff --git a/qapi-schema.json b/qapi-schema.json index 36cb964..06a9b5d 100644 --- a/qapi-schema.json +++ b/qapi-schema.json @@ -2099,8 +2099,9 @@ # @base: #optional The file name of the backing image to write data into. # If not specified, this is the deepest backing image # -# @top: The file name of the backing image within the image chain, -# which contains the topmost data to be committed down. +# @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. # # If top == base, that is an error. # If top == active, the job will not be completed by itself, @@ -2128,7 +2129,7 @@ # ## { 'command': 'block-commit', - 'data': { 'device': 'str', '*base': 'str', 'top': 'str', + 'data': { 'device': 'str', '*base': 'str', '*top': 'str', '*speed': 'int' } } ## diff --git a/qmp-commands.hx b/qmp-commands.hx index cae890e..1aa3c65 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?,top:s?,speed:o?", .mhandler.cmd_new = qmp_marshal_input_block_commit, }, @@ -1003,7 +1003,8 @@ Arguments: 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. + which contains the topmost data to be committed down. If + not specified, this is the active layer. (json-string, optional) If top == base, that is an error. If top == active, the job will not be completed by itself, diff --git a/tests/qemu-iotests/040 b/tests/qemu-iotests/040 index 734b6a6..803b0c7 100755 --- a/tests/qemu-iotests/040 +++ b/tests/qemu-iotests/040 @@ -35,11 +35,7 @@ test_img = os.path.join(iotests.test_dir, 'test.img') class ImageCommitTestCase(iotests.QMPTestCase): '''Abstract base class for image commit test cases''' - def run_commit_test(self, top, base): - self.assert_no_active_block_jobs() - result = self.vm.qmp('block-commit', device='drive0', top=top, base=base) - self.assert_qmp(result, 'return', {}) - + def wait_for_complete(self): completed = False while not completed: for event in self.vm.get_qmp_events(wait=True): @@ -58,6 +54,18 @@ class ImageCommitTestCase(iotests.QMPTestCase): self.assert_no_active_block_jobs() self.vm.shutdown() + def run_commit_test(self, top, base): + self.assert_no_active_block_jobs() + result = self.vm.qmp('block-commit', device='drive0', top=top, base=base) + self.assert_qmp(result, 'return', {}) + self.wait_for_complete() + + def run_default_commit_test(self): + self.assert_no_active_block_jobs() + result = self.vm.qmp('block-commit', device='drive0') + self.assert_qmp(result, 'return', {}) + self.wait_for_complete() + class TestSingleDrive(ImageCommitTestCase): image_len = 1 * 1024 * 1024 test_len = 1 * 1024 * 256 @@ -109,17 +117,17 @@ class TestSingleDrive(ImageCommitTestCase): self.assertEqual(-1, qemu_io('-c', 'read -P 0xab 0 524288', backing_img).find("verification failed")) self.assertEqual(-1, qemu_io('-c', 'read -P 0xef 524288 524288', backing_img).find("verification failed")) + def test_top_is_default_active(self): + self.run_default_commit_test() + self.assertEqual(-1, qemu_io('-c', 'read -P 0xab 0 524288', backing_img).find("verification failed")) + self.assertEqual(-1, qemu_io('-c', 'read -P 0xef 524288 524288', backing_img).find("verification failed")) + def test_top_and_base_reversed(self): self.assert_no_active_block_jobs() result = self.vm.qmp('block-commit', device='drive0', top='%s' % backing_img, base='%s' % mid_img) self.assert_qmp(result, 'error/class', 'GenericError') self.assert_qmp(result, 'error/desc', 'Base \'%s\' not found' % mid_img) - def test_top_omitted(self): - self.assert_no_active_block_jobs() - result = self.vm.qmp('block-commit', device='drive0') - self.assert_qmp(result, 'error/class', 'GenericError') - self.assert_qmp(result, 'error/desc', "Parameter 'top' is missing") class TestRelativePaths(ImageCommitTestCase): image_len = 1 * 1024 * 1024
Now that active layer block-commit is supported, the 'top' argument no longer needs to be mandatory. Change it optional, with the default being the active layer in the device chain. Signed-off-by: Jeff Cody <jcody@redhat.com> --- blockdev.c | 3 ++- qapi-schema.json | 7 ++++--- qmp-commands.hx | 5 +++-- tests/qemu-iotests/040 | 28 ++++++++++++++++++---------- 4 files changed, 27 insertions(+), 16 deletions(-)