[Qemu-devel] [PATCH v3 0/2] block/drive-mirror: Check for NULL backing_hd

[Qemu-devel] [PATCH v3 0/2] block/drive-mirror: Check for NULL backing_hd

[Max Reitz]

It should be possible to execute the QMP "drive-mirror" command in
"none" sync mode and "absolute-paths" mode even for block devices
lacking a backing file.

"absolute-paths" does in fact not require a backing file to be present,
as can be seen from the "top" sync mode code path. "top" basically
states that the device should indeed have a backing file - however, the
current code catches the case if it doesn't and then simply treats it as
"full" sync mode, creating a target image without a backing file (in
"absolute-paths" mode). Thus, "absolute-paths" does not imply the target
file must indeed have a backing file.

Therefore, the target file may be left unbacked in case of "none" sync
mode as well, if the specified device is not backed either. Currently,
qemu will crash trying to dereference the backing file pointer since it
assumes that it will always be non-NULL in that case ("none" with
"absolute-paths").

The first patch in this series adds a check whether the specified block
device is backed or not (creating an unbacked target image, if required);
the second patch adds a test case for mirroring unbacked block devices.

v2:
 - patch 1: Reuse an already existing codepath to create an unbacked
            target image instead of introducing a new one (based on
            Fam's comment).
 - patch 2: Incorporated test case into 041 instead of creating a new
            file (according to Xia's and Paolo's comments).


Max Reitz (2):
  block/drive-mirror: Check for NULL backing_hd
  qemu-iotests: Extend 041 for unbacked mirroring

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

-- 
1.8.4.2

[Qemu-devel] [PATCH v3 1/2] block/drive-mirror: Check for NULL backing_hd

[Max Reitz]

It should be possible to execute the QMP "drive-mirror" command in
"none" sync mode and "absolute-paths" mode even for block devices
lacking a backing file.

"absolute-paths" does in fact not require a backing file to be present,
as can be seen from the "top" sync mode code path. "top" basically
states that the device should indeed have a backing file - however, the
current code catches the case if it doesn't and then simply treats it as
"full" sync mode, creating a target image without a backing file (in
"absolute-paths" mode). Thus, "absolute-paths" does not imply the target
file must indeed have a backing file.

Therefore, the target file may be left unbacked in case of "none" sync
mode as well, if the specified device is not backed either. Currently,
qemu will crash trying to dereference the backing file pointer since it
assumes that it will always be non-NULL in that case ("none" with
"absolute-paths").

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

diff --git a/blockdev.c b/blockdev.c
index b260477..1c426b0 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -2026,7 +2026,9 @@ void qmp_drive_mirror(const char *device, const char *target,
         return;
     }
 
-    if (sync == MIRROR_SYNC_MODE_FULL && mode != NEW_IMAGE_MODE_EXISTING) {
+    if ((sync == MIRROR_SYNC_MODE_FULL || !source)
+        && mode != NEW_IMAGE_MODE_EXISTING)
+    {
         /* create new image w/o backing file */
         assert(format && drv);
         bdrv_img_create(target, format,
-- 
1.8.4.2

[Qemu-devel] [PATCH v3 2/2] qemu-iotests: Extend 041 for unbacked mirroring

[Max Reitz]

Add a new test case in file 041 for mirroring unbacked images in
"absolute-paths" mode. This should work, if possible, but most
importantly, qemu should never crash.

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

diff --git a/tests/qemu-iotests/041 b/tests/qemu-iotests/041
index 6661c03..5d40265 100755
--- a/tests/qemu-iotests/041
+++ b/tests/qemu-iotests/041
@@ -677,5 +677,30 @@ class TestSetSpeed(ImageMirroringTestCase):
 
         self.wait_ready_and_cancel()
 
+class TestUnbackedSource(ImageMirroringTestCase):
+    image_len = 2 * 1024 * 1024 # MB
+
+    def setUp(self):
+        qemu_img('create', '-f', iotests.imgfmt, test_img,
+                 str(TestUnbackedSource.image_len))
+        self.vm = iotests.VM().add_drive(test_img)
+        self.vm.launch()
+
+    def tearDown(self):
+        self.vm.shutdown()
+        os.remove(test_img)
+        os.remove(target_img)
+
+    def test_absolute_paths(self):
+        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()
+
 if __name__ == '__main__':
     iotests.main(supported_fmts=['qcow2', 'qed'])
diff --git a/tests/qemu-iotests/041.out b/tests/qemu-iotests/041.out
index 42314e9..4fd1c2d 100644
--- a/tests/qemu-iotests/041.out
+++ b/tests/qemu-iotests/041.out
@@ -1,5 +1,5 @@
-........................
+.........................
 ----------------------------------------------------------------------
-Ran 24 tests
+Ran 25 tests
 
 OK
-- 
1.8.4.2

Re: [Qemu-devel] [PATCH v3 1/2] block/drive-mirror: Check for NULL backing_hd

[Eric Blake]

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

On 11/06/2013 11:50 AM, Max Reitz wrote:
> It should be possible to execute the QMP "drive-mirror" command in
> "none" sync mode and "absolute-paths" mode even for block devices
> lacking a backing file.
> 
> "absolute-paths" does in fact not require a backing file to be present,
> as can be seen from the "top" sync mode code path. "top" basically
> states that the device should indeed have a backing file - however, the
> current code catches the case if it doesn't and then simply treats it as
> "full" sync mode, creating a target image without a backing file (in
> "absolute-paths" mode). Thus, "absolute-paths" does not imply the target
> file must indeed have a backing file.
> 
> Therefore, the target file may be left unbacked in case of "none" sync
> mode as well, if the specified device is not backed either. Currently,
> qemu will crash trying to dereference the backing file pointer since it
> assumes that it will always be non-NULL in that case ("none" with
> "absolute-paths").
> 
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
>  blockdev.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)

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

> 
> diff --git a/blockdev.c b/blockdev.c
> index b260477..1c426b0 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -2026,7 +2026,9 @@ void qmp_drive_mirror(const char *device, const char *target,
>          return;
>      }
>  
> -    if (sync == MIRROR_SYNC_MODE_FULL && mode != NEW_IMAGE_MODE_EXISTING) {
> +    if ((sync == MIRROR_SYNC_MODE_FULL || !source)
> +        && mode != NEW_IMAGE_MODE_EXISTING)
> +    {
>          /* create new image w/o backing file */
>          assert(format && drv);
>          bdrv_img_create(target, format,
> 

-- 
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 v3 0/2] block/drive-mirror: Check for NULL backing_hd

[Wenchao Xia]

于 2013/11/7 2:50, Max Reitz 写道:
> It should be possible to execute the QMP "drive-mirror" command in
> "none" sync mode and "absolute-paths" mode even for block devices
> lacking a backing file.
> 
> "absolute-paths" does in fact not require a backing file to be present,
> as can be seen from the "top" sync mode code path. "top" basically
> states that the device should indeed have a backing file - however, the
> current code catches the case if it doesn't and then simply treats it as
> "full" sync mode, creating a target image without a backing file (in
> "absolute-paths" mode). Thus, "absolute-paths" does not imply the target
> file must indeed have a backing file.
> 
> Therefore, the target file may be left unbacked in case of "none" sync
> mode as well, if the specified device is not backed either. Currently,
> qemu will crash trying to dereference the backing file pointer since it
> assumes that it will always be non-NULL in that case ("none" with
> "absolute-paths").
> 
> The first patch in this series adds a check whether the specified block
> device is backed or not (creating an unbacked target image, if required);
> the second patch adds a test case for mirroring unbacked block devices.
> 
> v2:
>   - patch 1: Reuse an already existing codepath to create an unbacked
>              target image instead of introducing a new one (based on
>              Fam's comment).
>   - patch 2: Incorporated test case into 041 instead of creating a new
>              file (according to Xia's and Paolo's comments).
> 
> 
> Max Reitz (2):
>    block/drive-mirror: Check for NULL backing_hd
>    qemu-iotests: Extend 041 for unbacked mirroring
> 
>   blockdev.c                 |  4 +++-
>   tests/qemu-iotests/041     | 25 +++++++++++++++++++++++++
>   tests/qemu-iotests/041.out |  4 ++--
>   3 files changed, 30 insertions(+), 3 deletions(-)
> 

Reviewed-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>

Should we change doc for NewImageMode in qapi-schema.json also?

Re: [Qemu-devel] [PATCH v3 0/2] block/drive-mirror: Check for NULL backing_hd

[Max Reitz]

On 07.11.2013 03:25, Wenchao Xia wrote:
> 于 2013/11/7 2:50, Max Reitz 写道:
>> It should be possible to execute the QMP "drive-mirror" command in
>> "none" sync mode and "absolute-paths" mode even for block devices
>> lacking a backing file.
>>
>> "absolute-paths" does in fact not require a backing file to be present,
>> as can be seen from the "top" sync mode code path. "top" basically
>> states that the device should indeed have a backing file - however, the
>> current code catches the case if it doesn't and then simply treats it as
>> "full" sync mode, creating a target image without a backing file (in
>> "absolute-paths" mode). Thus, "absolute-paths" does not imply the target
>> file must indeed have a backing file.
>>
>> Therefore, the target file may be left unbacked in case of "none" sync
>> mode as well, if the specified device is not backed either. Currently,
>> qemu will crash trying to dereference the backing file pointer since it
>> assumes that it will always be non-NULL in that case ("none" with
>> "absolute-paths").
>>
>> The first patch in this series adds a check whether the specified block
>> device is backed or not (creating an unbacked target image, if required);
>> the second patch adds a test case for mirroring unbacked block devices.
>>
>> v2:
>>   - patch 1: Reuse an already existing codepath to create an unbacked
>>              target image instead of introducing a new one (based on
>>              Fam's comment).
>>   - patch 2: Incorporated test case into 041 instead of creating a new
>>              file (according to Xia's and Paolo's comments).
>>
>>
>> Max Reitz (2):
>>    block/drive-mirror: Check for NULL backing_hd
>>    qemu-iotests: Extend 041 for unbacked mirroring
>>
>>   blockdev.c                 |  4 +++-
>>   tests/qemu-iotests/041     | 25 +++++++++++++++++++++++++
>>   tests/qemu-iotests/041.out |  4 ++--
>>   3 files changed, 30 insertions(+), 3 deletions(-)
>>
> Reviewed-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
>
> Should we change doc for NewImageMode in qapi-schema.json also?

Probably, yes. I'll send a follow-up patch.

Max

Re: [Qemu-devel] [PATCH v3 2/2] qemu-iotests: Extend 041 for unbacked mirroring

[Paolo Bonzini]

Il 06/11/2013 19:50, Max Reitz ha scritto:
> Add a new test case in file 041 for mirroring unbacked images in
> "absolute-paths" mode. This should work, if possible, but most
> importantly, qemu should never crash.
> 
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
>  tests/qemu-iotests/041     | 25 +++++++++++++++++++++++++
>  tests/qemu-iotests/041.out |  4 ++--
>  2 files changed, 27 insertions(+), 2 deletions(-)
> 
> diff --git a/tests/qemu-iotests/041 b/tests/qemu-iotests/041
> index 6661c03..5d40265 100755
> --- a/tests/qemu-iotests/041
> +++ b/tests/qemu-iotests/041
> @@ -677,5 +677,30 @@ class TestSetSpeed(ImageMirroringTestCase):
>  
>          self.wait_ready_and_cancel()
>  
> +class TestUnbackedSource(ImageMirroringTestCase):
> +    image_len = 2 * 1024 * 1024 # MB
> +
> +    def setUp(self):
> +        qemu_img('create', '-f', iotests.imgfmt, test_img,
> +                 str(TestUnbackedSource.image_len))
> +        self.vm = iotests.VM().add_drive(test_img)
> +        self.vm.launch()
> +
> +    def tearDown(self):
> +        self.vm.shutdown()
> +        os.remove(test_img)
> +        os.remove(target_img)
> +
> +    def test_absolute_paths(self):
> +        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()
> +
>  if __name__ == '__main__':
>      iotests.main(supported_fmts=['qcow2', 'qed'])
> diff --git a/tests/qemu-iotests/041.out b/tests/qemu-iotests/041.out
> index 42314e9..4fd1c2d 100644
> --- a/tests/qemu-iotests/041.out
> +++ b/tests/qemu-iotests/041.out
> @@ -1,5 +1,5 @@
> -........................
> +.........................
>  ----------------------------------------------------------------------
> -Ran 24 tests
> +Ran 25 tests
>  
>  OK
> 

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

Paolo

Re: [Qemu-devel] [PATCH v3 0/2] block/drive-mirror: Check for NULL backing_hd

[Kevin Wolf]

Am 06.11.2013 um 19:50 hat Max Reitz geschrieben:
> It should be possible to execute the QMP "drive-mirror" command in
> "none" sync mode and "absolute-paths" mode even for block devices
> lacking a backing file.
> 
> "absolute-paths" does in fact not require a backing file to be present,
> as can be seen from the "top" sync mode code path. "top" basically
> states that the device should indeed have a backing file - however, the
> current code catches the case if it doesn't and then simply treats it as
> "full" sync mode, creating a target image without a backing file (in
> "absolute-paths" mode). Thus, "absolute-paths" does not imply the target
> file must indeed have a backing file.
> 
> Therefore, the target file may be left unbacked in case of "none" sync
> mode as well, if the specified device is not backed either. Currently,
> qemu will crash trying to dereference the backing file pointer since it
> assumes that it will always be non-NULL in that case ("none" with
> "absolute-paths").
> 
> The first patch in this series adds a check whether the specified block
> device is backed or not (creating an unbacked target image, if required);
> the second patch adds a test case for mirroring unbacked block devices.
> 
> v2:
>  - patch 1: Reuse an already existing codepath to create an unbacked
>             target image instead of introducing a new one (based on
>             Fam's comment).
>  - patch 2: Incorporated test case into 041 instead of creating a new
>             file (according to Xia's and Paolo's comments).
> 
> 
> Max Reitz (2):
>   block/drive-mirror: Check for NULL backing_hd
>   qemu-iotests: Extend 041 for unbacked mirroring
> 
>  blockdev.c                 |  4 +++-
>  tests/qemu-iotests/041     | 25 +++++++++++++++++++++++++
>  tests/qemu-iotests/041.out |  4 ++--
>  3 files changed, 30 insertions(+), 3 deletions(-)

Thanks, applied to the block branch.

Kevin