[Qemu-devel] [PATCH for-1.7 0/2] block/drive-mirror: Reuse backing HD for sync=none

[Qemu-devel] [PATCH for-1.7 0/2] block/drive-mirror: Reuse backing HD for sync=none

[Max Reitz]

This series fixes the drive-mirror blockjob in case of "none" sync mode
to always use the old (current) image file as the backing file of the
newly created mirrored file (in case of "absolute-paths" mode).

It is rather important to get this into 1.7, as we will introduce an at
least pretty strange API in case the original file is unbacked
otherwise.


Max Reitz (2):
  block/drive-mirror: Reuse backing HD for sync=none
  qemu-iotests: Fix test 041

 blockdev.c                 |  3 +++
 tests/qemu-iotests/041     | 32 ++++++++++++++++++++++++--------
 tests/qemu-iotests/041.out |  4 ++--
 3 files changed, 29 insertions(+), 10 deletions(-)

-- 
1.8.4.2

[Qemu-devel] [PATCH for-1.7 1/2] block/drive-mirror: Reuse backing HD for sync=none

[Max Reitz]

For "none" sync mode in "absolute-paths" mode, the current image should
be used as the backing file for the newly created image.

The current behavior is:
a) If the image to be mirrored has a backing file, use that (which is
   wrong, since the operations recorded by "none" are applied to the
   image itself, not to its backing file).
b) If the image to be mirrored lacks a backing file, the target doesn't
   have one either (which is not really wrong, but not really right,
   either; "none" records a set of operations executed on the image
   file, therefore having no backing file to apply these operations on
   seems rather pointless).

For a, this is clearly a bugfix. For b, it is still a bugfix, although
it might break existing API - but since that case crashed qemu just
three weeks ago (before 1452686495922b81d6cf43edf025c1aef15965c0), we
can safely assume there is no such API relying on that case yet.

Suggested-by: Paolo Bonzini <pbonzini@redhat.com>
Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 blockdev.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/blockdev.c b/blockdev.c
index 330aa4a..44755e1 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -2021,6 +2021,9 @@ void qmp_drive_mirror(const char *device, const char *target,
     if (!source && sync == MIRROR_SYNC_MODE_TOP) {
         sync = MIRROR_SYNC_MODE_FULL;
     }
+    if (sync == MIRROR_SYNC_MODE_NONE) {
+        source = bs;
+    }
 
     size = bdrv_getlength(bs);
     if (size < 0) {
-- 
1.8.4.2

[Qemu-devel] [PATCH for-1.7 2/2] qemu-iotests: Fix test 041

[Max Reitz]

Performing multiple drive-mirror blockjobs on the same qemu instance
results in the image file used for the block device being replaced by
the newly mirrored file, which is not what we want.

Fix this by performing one dedicated test per sync mode.

Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 tests/qemu-iotests/041     | 32 ++++++++++++++++++++++++--------
 tests/qemu-iotests/041.out |  4 ++--
 2 files changed, 26 insertions(+), 10 deletions(-)

diff --git a/tests/qemu-iotests/041 b/tests/qemu-iotests/041
index 5d40265..ec470b2 100755
--- a/tests/qemu-iotests/041
+++ b/tests/qemu-iotests/041
@@ -691,16 +691,32 @@ class TestUnbackedSource(ImageMirroringTestCase):
         os.remove(test_img)
         os.remove(target_img)
 
-    def test_absolute_paths(self):
+    def test_absolute_paths_full(self):
+        self.assert_no_active_block_jobs()
+        result = self.vm.qmp('drive-mirror', device='drive0',
+                             sync='full', target=target_img,
+                             mode='absolute-paths')
+        self.assert_qmp(result, 'return', {})
+        self.complete_and_wait()
+        self.assert_no_active_block_jobs()
+
+    def test_absolute_paths_top(self):
+        self.assert_no_active_block_jobs()
+        result = self.vm.qmp('drive-mirror', device='drive0',
+                             sync='top', target=target_img,
+                             mode='absolute-paths')
+        self.assert_qmp(result, 'return', {})
+        self.complete_and_wait()
         self.assert_no_active_block_jobs()
 
-        for sync_mode in ['full', 'top', 'none']:
-            result = self.vm.qmp('drive-mirror', device='drive0',
-                                 sync=sync_mode, target=target_img,
-                                 mode='absolute-paths')
-            self.assert_qmp(result, 'return', {})
-            self.complete_and_wait()
-            self.assert_no_active_block_jobs()
+    def test_absolute_paths_none(self):
+        self.assert_no_active_block_jobs()
+        result = self.vm.qmp('drive-mirror', device='drive0',
+                             sync='none', target=target_img,
+                             mode='absolute-paths')
+        self.assert_qmp(result, 'return', {})
+        self.complete_and_wait()
+        self.assert_no_active_block_jobs()
 
 if __name__ == '__main__':
     iotests.main(supported_fmts=['qcow2', 'qed'])
diff --git a/tests/qemu-iotests/041.out b/tests/qemu-iotests/041.out
index 4fd1c2d..6d9bee1 100644
--- a/tests/qemu-iotests/041.out
+++ b/tests/qemu-iotests/041.out
@@ -1,5 +1,5 @@
-.........................
+...........................
 ----------------------------------------------------------------------
-Ran 25 tests
+Ran 27 tests
 
 OK
-- 
1.8.4.2

Re: [Qemu-devel] [PATCH for-1.7 1/2] block/drive-mirror: Reuse backing HD for sync=none

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1513 bytes --]

On 11/25/2013 12:28 PM, Max Reitz wrote:
> For "none" sync mode in "absolute-paths" mode, the current image should
> be used as the backing file for the newly created image.
> 
> The current behavior is:
> a) If the image to be mirrored has a backing file, use that (which is
>    wrong, since the operations recorded by "none" are applied to the
>    image itself, not to its backing file).
> b) If the image to be mirrored lacks a backing file, the target doesn't
>    have one either (which is not really wrong, but not really right,
>    either; "none" records a set of operations executed on the image
>    file, therefore having no backing file to apply these operations on
>    seems rather pointless).
> 
> For a, this is clearly a bugfix. For b, it is still a bugfix, although
> it might break existing API - but since that case crashed qemu just
> three weeks ago (before 1452686495922b81d6cf43edf025c1aef15965c0), we
> can safely assume there is no such API relying on that case yet.
> 
> Suggested-by: Paolo Bonzini <pbonzini@redhat.com>
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
>  blockdev.c | 3 +++
>  1 file changed, 3 insertions(+)

Reviewed-by: Eric Blake <eblake@redhat.com>

And definitely agree that this is a bug fix worth having in 1.7, before
we bake in borked semantics that libvirt would have to work around
depending on qemu version.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 621 bytes --]

Re: [Qemu-devel] [PATCH for-1.7 2/2] qemu-iotests: Fix test 041

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 688 bytes --]

On 11/25/2013 12:28 PM, Max Reitz wrote:
> Performing multiple drive-mirror blockjobs on the same qemu instance
> results in the image file used for the block device being replaced by
> the newly mirrored file, which is not what we want.
> 
> Fix this by performing one dedicated test per sync mode.
> 
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
>  tests/qemu-iotests/041     | 32 ++++++++++++++++++++++++--------
>  tests/qemu-iotests/041.out |  4 ++--
>  2 files changed, 26 insertions(+), 10 deletions(-)

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 621 bytes --]

Re: [Qemu-devel] [PATCH for-1.7 0/2] block/drive-mirror: Reuse backing HD for sync=none

[Paolo Bonzini]

Il 25/11/2013 20:28, Max Reitz ha scritto:
> This series fixes the drive-mirror blockjob in case of "none" sync mode
> to always use the old (current) image file as the backing file of the
> newly created mirrored file (in case of "absolute-paths" mode).
> 
> It is rather important to get this into 1.7, as we will introduce an at
> least pretty strange API in case the original file is unbacked
> otherwise.
> 
> 
> Max Reitz (2):
>   block/drive-mirror: Reuse backing HD for sync=none
>   qemu-iotests: Fix test 041

Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>

Re: [Qemu-devel] [PATCH for-1.7 0/2] block/drive-mirror: Reuse backing HD for sync=none

[Anthony Liguori]

Max Reitz <mreitz@redhat.com> writes:

> This series fixes the drive-mirror blockjob in case of "none" sync mode
> to always use the old (current) image file as the backing file of the
> newly created mirrored file (in case of "absolute-paths" mode).
>
> It is rather important to get this into 1.7, as we will introduce an at
> least pretty strange API in case the original file is unbacked
> otherwise.

Kevin/Stefan?  Do we need this for 1.7?

Regards,

Anthony Liguori

>
>
> Max Reitz (2):
>   block/drive-mirror: Reuse backing HD for sync=none
>   qemu-iotests: Fix test 041
>
>  blockdev.c                 |  3 +++
>  tests/qemu-iotests/041     | 32 ++++++++++++++++++++++++--------
>  tests/qemu-iotests/041.out |  4 ++--
>  3 files changed, 29 insertions(+), 10 deletions(-)
>
> -- 
> 1.8.4.2

Re: [Qemu-devel] [PATCH for-1.7 0/2] block/drive-mirror: Reuse backing HD for sync=none

[Kevin Wolf]

Am 26.11.2013 um 19:02 hat Anthony Liguori geschrieben:
> Max Reitz <mreitz@redhat.com> writes:
> 
> > This series fixes the drive-mirror blockjob in case of "none" sync mode
> > to always use the old (current) image file as the backing file of the
> > newly created mirrored file (in case of "absolute-paths" mode).
> >
> > It is rather important to get this into 1.7, as we will introduce an at
> > least pretty strange API in case the original file is unbacked
> > otherwise.
> 
> Kevin/Stefan?  Do we need this for 1.7?

Yes, it would be good to pick it up in order to avoid changing the API
in 1.8 (I guess we would do it anyway because the current behaviour
doesn't make any sense and we'd call it bug fix, but libvirt would have
to deal with it and better to do it right in the first release.)

Do you want to pick it up yourself or should I send a pull request?

Reviewed-by: Kevin Wolf <kwolf@redhat.com>

Kevin

Re: [Qemu-devel] [PATCH for-1.7 0/2] block/drive-mirror: Reuse backing HD for sync=none

[Anthony Liguori]

Kevin Wolf <kwolf@redhat.com> writes:

> Am 26.11.2013 um 19:02 hat Anthony Liguori geschrieben:
>> Max Reitz <mreitz@redhat.com> writes:
>> 
>> > This series fixes the drive-mirror blockjob in case of "none" sync mode
>> > to always use the old (current) image file as the backing file of the
>> > newly created mirrored file (in case of "absolute-paths" mode).
>> >
>> > It is rather important to get this into 1.7, as we will introduce an at
>> > least pretty strange API in case the original file is unbacked
>> > otherwise.
>> 
>> Kevin/Stefan?  Do we need this for 1.7?
>
> Yes, it would be good to pick it up in order to avoid changing the API
> in 1.8 (I guess we would do it anyway because the current behaviour
> doesn't make any sense and we'd call it bug fix, but libvirt would have
> to deal with it and better to do it right in the first release.)
>
> Do you want to pick it up yourself or should I send a pull request?

I'll pick it up.

Regards,

Anthony Liguori

>
> Reviewed-by: Kevin Wolf <kwolf@redhat.com>
>
> Kevin