[Qemu-devel] [PATCH] block: change default of .has_zero_init to 0

[Qemu-devel] [PATCH] block: change default of .has_zero_init to 0

[Peter Lieven]

.has_zero_init defaults to 1 for all formats and protocols.

this is a dangerous default since this means that all
new added drivers need to manually overwrite it to 0 if
they do not ensure that a device is zero initialized
after bdrv_create().

if a driver needs to explicitly set this value to
1 its easier to verify the correctness in the review process.

during review of the existing drivers it turned out
that ssh and gluster had a wrong default of 1.
both protocols support host_devices as backend
which are not by default zero initialized. this
wrong assumption will lead to possible corruption
if qemu-img convert is used to write to such a backend.

a similar problem with the wrong default existed for
iscsi mose likely because the driver developer did
oversee the default value of 1.

Signed-off-by: Peter Lieven <pl@kamp.de>
---
 block.c               |    8 +++++++-
 block/qcow.c          |    1 +
 block/qcow2.c         |    1 +
 block/qed.c           |    1 +
 block/raw-posix.c     |   10 +---------
 block/raw-win32.c     |    7 +------
 block/rbd.c           |    1 +
 block/sheepdog.c      |    1 +
 block/vdi.c           |    1 +
 block/vmdk.c          |    1 +
 include/block/block.h |    1 +
 11 files changed, 17 insertions(+), 16 deletions(-)

diff --git a/block.c b/block.c
index 8e77d46..7b36fb9 100644
--- a/block.c
+++ b/block.c
@@ -2916,6 +2916,11 @@ void bdrv_flush_all(void)
     }
 }
 
+int bdrv_has_zero_init_1(BlockDriverState *bs)
+{
+    return 1;
+}
+
 int bdrv_has_zero_init(BlockDriverState *bs)
 {
     assert(bs->drv);
@@ -2924,7 +2929,8 @@ int bdrv_has_zero_init(BlockDriverState *bs)
         return bs->drv->bdrv_has_zero_init(bs);
     }
 
-    return 1;
+    /* save default */
+    return 0;
 }
 
 typedef struct BdrvCoIsAllocatedData {
diff --git a/block/qcow.c b/block/qcow.c
index e2a64c7..5239bd6 100644
--- a/block/qcow.c
+++ b/block/qcow.c
@@ -892,6 +892,7 @@ static BlockDriver bdrv_qcow = {
     .bdrv_close		= qcow_close,
     .bdrv_reopen_prepare = qcow_reopen_prepare,
     .bdrv_create	= qcow_create,
+    .bdrv_has_zero_init     = bdrv_has_zero_init_1,
 
     .bdrv_co_readv          = qcow_co_readv,
     .bdrv_co_writev         = qcow_co_writev,
diff --git a/block/qcow2.c b/block/qcow2.c
index 9383990..0eceefe 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -1785,6 +1785,7 @@ static BlockDriver bdrv_qcow2 = {
     .bdrv_close         = qcow2_close,
     .bdrv_reopen_prepare  = qcow2_reopen_prepare,
     .bdrv_create        = qcow2_create,
+    .bdrv_has_zero_init = bdrv_has_zero_init_1,
     .bdrv_co_is_allocated = qcow2_co_is_allocated,
     .bdrv_set_key       = qcow2_set_key,
     .bdrv_make_empty    = qcow2_make_empty,
diff --git a/block/qed.c b/block/qed.c
index 4651403..f767b05 100644
--- a/block/qed.c
+++ b/block/qed.c
@@ -1574,6 +1574,7 @@ static BlockDriver bdrv_qed = {
     .bdrv_close               = bdrv_qed_close,
     .bdrv_reopen_prepare      = bdrv_qed_reopen_prepare,
     .bdrv_create              = bdrv_qed_create,
+    .bdrv_has_zero_init       = bdrv_has_zero_init_1,
     .bdrv_co_is_allocated     = bdrv_qed_co_is_allocated,
     .bdrv_make_empty          = bdrv_qed_make_empty,
     .bdrv_aio_readv           = bdrv_qed_aio_readv,
diff --git a/block/raw-posix.c b/block/raw-posix.c
index c0ccf27..8597d77 100644
--- a/block/raw-posix.c
+++ b/block/raw-posix.c
@@ -1199,6 +1199,7 @@ static BlockDriver bdrv_file = {
     .bdrv_reopen_abort = raw_reopen_abort,
     .bdrv_close = raw_close,
     .bdrv_create = raw_create,
+    .bdrv_has_zero_init = bdrv_has_zero_init_1,
     .bdrv_co_is_allocated = raw_co_is_allocated,
 
     .bdrv_aio_readv = raw_aio_readv,
@@ -1526,11 +1527,6 @@ static int hdev_create(const char *filename, QEMUOptionParameter *options)
     return ret;
 }
 
-static int hdev_has_zero_init(BlockDriverState *bs)
-{
-    return 0;
-}
-
 static BlockDriver bdrv_host_device = {
     .format_name        = "host_device",
     .protocol_name        = "host_device",
@@ -1543,7 +1539,6 @@ static BlockDriver bdrv_host_device = {
     .bdrv_reopen_abort   = raw_reopen_abort,
     .bdrv_create        = hdev_create,
     .create_options     = raw_create_options,
-    .bdrv_has_zero_init = hdev_has_zero_init,
 
     .bdrv_aio_readv	= raw_aio_readv,
     .bdrv_aio_writev	= raw_aio_writev,
@@ -1668,7 +1663,6 @@ static BlockDriver bdrv_host_floppy = {
     .bdrv_reopen_abort   = raw_reopen_abort,
     .bdrv_create        = hdev_create,
     .create_options     = raw_create_options,
-    .bdrv_has_zero_init = hdev_has_zero_init,
 
     .bdrv_aio_readv     = raw_aio_readv,
     .bdrv_aio_writev    = raw_aio_writev,
@@ -1770,7 +1764,6 @@ static BlockDriver bdrv_host_cdrom = {
     .bdrv_reopen_abort   = raw_reopen_abort,
     .bdrv_create        = hdev_create,
     .create_options     = raw_create_options,
-    .bdrv_has_zero_init = hdev_has_zero_init,
 
     .bdrv_aio_readv     = raw_aio_readv,
     .bdrv_aio_writev    = raw_aio_writev,
@@ -1892,7 +1885,6 @@ static BlockDriver bdrv_host_cdrom = {
     .bdrv_reopen_abort   = raw_reopen_abort,
     .bdrv_create        = hdev_create,
     .create_options     = raw_create_options,
-    .bdrv_has_zero_init = hdev_has_zero_init,
 
     .bdrv_aio_readv     = raw_aio_readv,
     .bdrv_aio_writev    = raw_aio_writev,
diff --git a/block/raw-win32.c b/block/raw-win32.c
index 7c03b6d..9b5b2af 100644
--- a/block/raw-win32.c
+++ b/block/raw-win32.c
@@ -459,6 +459,7 @@ static BlockDriver bdrv_file = {
     .bdrv_file_open	= raw_open,
     .bdrv_close		= raw_close,
     .bdrv_create	= raw_create,
+    .bdrv_has_zero_init = bdrv_has_zero_init_1,
 
     .bdrv_aio_readv     = raw_aio_readv,
     .bdrv_aio_writev    = raw_aio_writev,
@@ -570,11 +571,6 @@ static int hdev_open(BlockDriverState *bs, QDict *options, int flags)
     return 0;
 }
 
-static int hdev_has_zero_init(BlockDriverState *bs)
-{
-    return 0;
-}
-
 static BlockDriver bdrv_host_device = {
     .format_name	= "host_device",
     .protocol_name	= "host_device",
@@ -582,7 +578,6 @@ static BlockDriver bdrv_host_device = {
     .bdrv_probe_device	= hdev_probe_device,
     .bdrv_file_open	= hdev_open,
     .bdrv_close		= raw_close,
-    .bdrv_has_zero_init = hdev_has_zero_init,
 
     .bdrv_aio_readv     = raw_aio_readv,
     .bdrv_aio_writev    = raw_aio_writev,
diff --git a/block/rbd.c b/block/rbd.c
index 0f2608b..cb71751 100644
--- a/block/rbd.c
+++ b/block/rbd.c
@@ -996,6 +996,7 @@ static BlockDriver bdrv_rbd = {
     .bdrv_file_open     = qemu_rbd_open,
     .bdrv_close         = qemu_rbd_close,
     .bdrv_create        = qemu_rbd_create,
+    .bdrv_has_zero_init = bdrv_has_zero_init_1,
     .bdrv_get_info      = qemu_rbd_getinfo,
     .create_options     = qemu_rbd_create_options,
     .bdrv_getlength     = qemu_rbd_getlength,
diff --git a/block/sheepdog.c b/block/sheepdog.c
index 1b7c3f1..b397b5b 100644
--- a/block/sheepdog.c
+++ b/block/sheepdog.c
@@ -2401,6 +2401,7 @@ static BlockDriver bdrv_sheepdog_unix = {
     .bdrv_file_open = sd_open,
     .bdrv_close     = sd_close,
     .bdrv_create    = sd_create,
+    .bdrv_has_zero_init = bdrv_has_zero_init_1,
     .bdrv_getlength = sd_getlength,
     .bdrv_truncate  = sd_truncate,
 
diff --git a/block/vdi.c b/block/vdi.c
index 2662d89..8a91525 100644
--- a/block/vdi.c
+++ b/block/vdi.c
@@ -779,6 +779,7 @@ static BlockDriver bdrv_vdi = {
     .bdrv_close = vdi_close,
     .bdrv_reopen_prepare = vdi_reopen_prepare,
     .bdrv_create = vdi_create,
+    .bdrv_has_zero_init = bdrv_has_zero_init_1,
     .bdrv_co_is_allocated = vdi_co_is_allocated,
     .bdrv_make_empty = vdi_make_empty,
 
diff --git a/block/vmdk.c b/block/vmdk.c
index 975e1d4..c45e2e2 100644
--- a/block/vmdk.c
+++ b/block/vmdk.c
@@ -1814,6 +1814,7 @@ static BlockDriver bdrv_vmdk = {
     .bdrv_co_write_zeroes = vmdk_co_write_zeroes,
     .bdrv_close     = vmdk_close,
     .bdrv_create    = vmdk_create,
+    .bdrv_has_zero_init = bdrv_has_zero_init_1,
     .bdrv_co_flush_to_disk  = vmdk_co_flush,
     .bdrv_co_is_allocated   = vmdk_co_is_allocated,
     .bdrv_get_allocated_file_size  = vmdk_get_allocated_file_size,
diff --git a/include/block/block.h b/include/block/block.h
index 2307f67..dd8eca1 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -272,6 +272,7 @@ void bdrv_drain_all(void);
 
 int bdrv_discard(BlockDriverState *bs, int64_t sector_num, int nb_sectors);
 int bdrv_co_discard(BlockDriverState *bs, int64_t sector_num, int nb_sectors);
+int bdrv_has_zero_init_1(BlockDriverState *bs);
 int bdrv_has_zero_init(BlockDriverState *bs);
 int bdrv_is_allocated(BlockDriverState *bs, int64_t sector_num, int nb_sectors,
                       int *pnum);
-- 
1.7.9.5

Re: [Qemu-devel] [PATCH] block: change default of .has_zero_init to 0

[Richard W.M. Jones]

On Thu, Jun 27, 2013 at 03:52:34PM +0200, Peter Lieven wrote:
> +    /* save default */

A minor thing, but s/save/safe/ :-)

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
virt-p2v converts physical machines to virtual machines.  Boot with a
live CD or over the network (PXE) and turn machines into KVM guests.
http://libguestfs.org/virt-v2v

Re: [Qemu-devel] [PATCH] block: change default of .has_zero_init to 0

[Eric Blake]

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

On 06/27/2013 07:52 AM, Peter Lieven wrote:
> .has_zero_init defaults to 1 for all formats and protocols.
> 
> this is a dangerous default since this means that all
> new added drivers need to manually overwrite it to 0 if
> they do not ensure that a device is zero initialized
> after bdrv_create().
> 
> if a driver needs to explicitly set this value to
> 1 its easier to verify the correctness in the review process.
> 
> during review of the existing drivers it turned out
> that ssh and gluster had a wrong default of 1.
> both protocols support host_devices as backend
> which are not by default zero initialized. this
> wrong assumption will lead to possible corruption
> if qemu-img convert is used to write to such a backend.
> 
> a similar problem with the wrong default existed for
> iscsi mose likely because the driver developer did

s/mose/most/

-- 
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] block: change default of .has_zero_init to 0

[Kevin Wolf]

Am 27.06.2013 um 15:52 hat Peter Lieven geschrieben:
> .has_zero_init defaults to 1 for all formats and protocols.
> 
> this is a dangerous default since this means that all
> new added drivers need to manually overwrite it to 0 if
> they do not ensure that a device is zero initialized
> after bdrv_create().
> 
> if a driver needs to explicitly set this value to
> 1 its easier to verify the correctness in the review process.
> 
> during review of the existing drivers it turned out
> that ssh and gluster had a wrong default of 1.
> both protocols support host_devices as backend
> which are not by default zero initialized. this
> wrong assumption will lead to possible corruption
> if qemu-img convert is used to write to such a backend.
> 
> a similar problem with the wrong default existed for
> iscsi mose likely because the driver developer did
> oversee the default value of 1.
> 
> Signed-off-by: Peter Lieven <pl@kamp.de>
> ---
>  block.c               |    8 +++++++-
>  block/qcow.c          |    1 +
>  block/qcow2.c         |    1 +
>  block/qed.c           |    1 +
>  block/raw-posix.c     |   10 +---------
>  block/raw-win32.c     |    7 +------
>  block/rbd.c           |    1 +
>  block/sheepdog.c      |    1 +
>  block/vdi.c           |    1 +
>  block/vmdk.c          |    1 +
>  include/block/block.h |    1 +
>  11 files changed, 17 insertions(+), 16 deletions(-)

You forgot cow, which is also a simple case that can be handled in this
patch.

vpc is still easy, but a bit more complicated than a constant return 1,
because it depends on the subformat whether a new image will inherit the
has_zero_init property from the underlying storage or whether it always
produces zeros (for VHD_FIXED type images, it's basically raw + footer).
I'll send a separate patch for this.

A similar situation exists for vmdk, I think, just that the difference
can be per extent there. I guess we need to return 0 if one of the
extents is flat and has an underlying storage returning 0. I'll leave
this part to Fam. For now, please remove the bdrv_has_zero_init_1 for
vmdk from this patch as it's unsafe.

Kevin

Re: [Qemu-devel] [PATCH] block: change default of .has_zero_init to 0

[Peter Lieven]

On 28.06.2013 10:06, Kevin Wolf wrote:
> Am 27.06.2013 um 15:52 hat Peter Lieven geschrieben:
>> .has_zero_init defaults to 1 for all formats and protocols.
>>
>> this is a dangerous default since this means that all
>> new added drivers need to manually overwrite it to 0 if
>> they do not ensure that a device is zero initialized
>> after bdrv_create().
>>
>> if a driver needs to explicitly set this value to
>> 1 its easier to verify the correctness in the review process.
>>
>> during review of the existing drivers it turned out
>> that ssh and gluster had a wrong default of 1.
>> both protocols support host_devices as backend
>> which are not by default zero initialized. this
>> wrong assumption will lead to possible corruption
>> if qemu-img convert is used to write to such a backend.
>>
>> a similar problem with the wrong default existed for
>> iscsi mose likely because the driver developer did
>> oversee the default value of 1.
>>
>> Signed-off-by: Peter Lieven <pl@kamp.de>
>> ---
>>   block.c               |    8 +++++++-
>>   block/qcow.c          |    1 +
>>   block/qcow2.c         |    1 +
>>   block/qed.c           |    1 +
>>   block/raw-posix.c     |   10 +---------
>>   block/raw-win32.c     |    7 +------
>>   block/rbd.c           |    1 +
>>   block/sheepdog.c      |    1 +
>>   block/vdi.c           |    1 +
>>   block/vmdk.c          |    1 +
>>   include/block/block.h |    1 +
>>   11 files changed, 17 insertions(+), 16 deletions(-)
> You forgot cow, which is also a simple case that can be handled in this
> patch.
ups.
>
> vpc is still easy, but a bit more complicated than a constant return 1,
> because it depends on the subformat whether a new image will inherit the
> has_zero_init property from the underlying storage or whether it always
> produces zeros (for VHD_FIXED type images, it's basically raw + footer).
> I'll send a separate patch for this.
shall I leave this to the new 0 default until your patch is ready?
>
> A similar situation exists for vmdk, I think, just that the difference
> can be per extent there. I guess we need to return 0 if one of the
> extents is flat and has an underlying storage returning 0. I'll leave
> this part to Fam. For now, please remove the bdrv_has_zero_init_1 for
> vmdk from this patch as it's unsafe.
ok I will mention that the value is changed for vmdk (and vpc) in the
commit message.

thanks
peter

Re: [Qemu-devel] [PATCH] block: change default of .has_zero_init to 0

[Kevin Wolf]

Am 28.06.2013 um 10:16 hat Peter Lieven geschrieben:
> On 28.06.2013 10:06, Kevin Wolf wrote:
> >Am 27.06.2013 um 15:52 hat Peter Lieven geschrieben:
> >>.has_zero_init defaults to 1 for all formats and protocols.
> >>
> >>this is a dangerous default since this means that all
> >>new added drivers need to manually overwrite it to 0 if
> >>they do not ensure that a device is zero initialized
> >>after bdrv_create().
> >>
> >>if a driver needs to explicitly set this value to
> >>1 its easier to verify the correctness in the review process.
> >>
> >>during review of the existing drivers it turned out
> >>that ssh and gluster had a wrong default of 1.
> >>both protocols support host_devices as backend
> >>which are not by default zero initialized. this
> >>wrong assumption will lead to possible corruption
> >>if qemu-img convert is used to write to such a backend.
> >>
> >>a similar problem with the wrong default existed for
> >>iscsi mose likely because the driver developer did
> >>oversee the default value of 1.
> >>
> >>Signed-off-by: Peter Lieven <pl@kamp.de>
> >>---
> >>  block.c               |    8 +++++++-
> >>  block/qcow.c          |    1 +
> >>  block/qcow2.c         |    1 +
> >>  block/qed.c           |    1 +
> >>  block/raw-posix.c     |   10 +---------
> >>  block/raw-win32.c     |    7 +------
> >>  block/rbd.c           |    1 +
> >>  block/sheepdog.c      |    1 +
> >>  block/vdi.c           |    1 +
> >>  block/vmdk.c          |    1 +
> >>  include/block/block.h |    1 +
> >>  11 files changed, 17 insertions(+), 16 deletions(-)
> >You forgot cow, which is also a simple case that can be handled in this
> >patch.
> ups.
> >
> >vpc is still easy, but a bit more complicated than a constant return 1,
> >because it depends on the subformat whether a new image will inherit the
> >has_zero_init property from the underlying storage or whether it always
> >produces zeros (for VHD_FIXED type images, it's basically raw + footer).
> >I'll send a separate patch for this.
> shall I leave this to the new 0 default until your patch is ready?

Yes, please leave vpc alone. I guess my patch will be queued before
yours anyway. ;-)

> >A similar situation exists for vmdk, I think, just that the difference
> >can be per extent there. I guess we need to return 0 if one of the
> >extents is flat and has an underlying storage returning 0. I'll leave
> >this part to Fam. For now, please remove the bdrv_has_zero_init_1 for
> >vmdk from this patch as it's unsafe.
> ok I will mention that the value is changed for vmdk (and vpc) in the
> commit message.

Good idea for vmdk. For vpc, don't mention it, because see above.

Kevin

Re: [Qemu-devel] [PATCH] block: change default of .has_zero_init to 0

[Peter Lieven]

On 28.06.2013 10:20, Kevin Wolf wrote:
> Am 28.06.2013 um 10:16 hat Peter Lieven geschrieben:
>> On 28.06.2013 10:06, Kevin Wolf wrote:
>>> Am 27.06.2013 um 15:52 hat Peter Lieven geschrieben:
>>>> .has_zero_init defaults to 1 for all formats and protocols.
>>>>
>>>> this is a dangerous default since this means that all
>>>> new added drivers need to manually overwrite it to 0 if
>>>> they do not ensure that a device is zero initialized
>>>> after bdrv_create().
>>>>
>>>> if a driver needs to explicitly set this value to
>>>> 1 its easier to verify the correctness in the review process.
>>>>
>>>> during review of the existing drivers it turned out
>>>> that ssh and gluster had a wrong default of 1.
>>>> both protocols support host_devices as backend
>>>> which are not by default zero initialized. this
>>>> wrong assumption will lead to possible corruption
>>>> if qemu-img convert is used to write to such a backend.
>>>>
>>>> a similar problem with the wrong default existed for
>>>> iscsi mose likely because the driver developer did
>>>> oversee the default value of 1.
>>>>
>>>> Signed-off-by: Peter Lieven <pl@kamp.de>
>>>> ---
>>>>   block.c               |    8 +++++++-
>>>>   block/qcow.c          |    1 +
>>>>   block/qcow2.c         |    1 +
>>>>   block/qed.c           |    1 +
>>>>   block/raw-posix.c     |   10 +---------
>>>>   block/raw-win32.c     |    7 +------
>>>>   block/rbd.c           |    1 +
>>>>   block/sheepdog.c      |    1 +
>>>>   block/vdi.c           |    1 +
>>>>   block/vmdk.c          |    1 +
>>>>   include/block/block.h |    1 +
>>>>   11 files changed, 17 insertions(+), 16 deletions(-)
>>> You forgot cow, which is also a simple case that can be handled in this
>>> patch.
>> ups.
>>> vpc is still easy, but a bit more complicated than a constant return 1,
>>> because it depends on the subformat whether a new image will inherit the
>>> has_zero_init property from the underlying storage or whether it always
>>> produces zeros (for VHD_FIXED type images, it's basically raw + footer).
>>> I'll send a separate patch for this.
>> shall I leave this to the new 0 default until your patch is ready?
> Yes, please leave vpc alone. I guess my patch will be queued before
> yours anyway. ;-)
once v2 is ready and noone has objections maybe you can put it in your queue? ;-)

Peter

Re: [Qemu-devel] [PATCH] block: change default of .has_zero_init to 0

[Kevin Wolf]

Am 28.06.2013 um 10:25 hat Peter Lieven geschrieben:
> On 28.06.2013 10:20, Kevin Wolf wrote:
> >Am 28.06.2013 um 10:16 hat Peter Lieven geschrieben:
> >>On 28.06.2013 10:06, Kevin Wolf wrote:
> >>>Am 27.06.2013 um 15:52 hat Peter Lieven geschrieben:
> >>>>.has_zero_init defaults to 1 for all formats and protocols.
> >>>>
> >>>>this is a dangerous default since this means that all
> >>>>new added drivers need to manually overwrite it to 0 if
> >>>>they do not ensure that a device is zero initialized
> >>>>after bdrv_create().
> >>>>
> >>>>if a driver needs to explicitly set this value to
> >>>>1 its easier to verify the correctness in the review process.
> >>>>
> >>>>during review of the existing drivers it turned out
> >>>>that ssh and gluster had a wrong default of 1.
> >>>>both protocols support host_devices as backend
> >>>>which are not by default zero initialized. this
> >>>>wrong assumption will lead to possible corruption
> >>>>if qemu-img convert is used to write to such a backend.
> >>>>
> >>>>a similar problem with the wrong default existed for
> >>>>iscsi mose likely because the driver developer did
> >>>>oversee the default value of 1.
> >>>>
> >>>>Signed-off-by: Peter Lieven <pl@kamp.de>
> >>>>---
> >>>>  block.c               |    8 +++++++-
> >>>>  block/qcow.c          |    1 +
> >>>>  block/qcow2.c         |    1 +
> >>>>  block/qed.c           |    1 +
> >>>>  block/raw-posix.c     |   10 +---------
> >>>>  block/raw-win32.c     |    7 +------
> >>>>  block/rbd.c           |    1 +
> >>>>  block/sheepdog.c      |    1 +
> >>>>  block/vdi.c           |    1 +
> >>>>  block/vmdk.c          |    1 +
> >>>>  include/block/block.h |    1 +
> >>>>  11 files changed, 17 insertions(+), 16 deletions(-)
> >>>You forgot cow, which is also a simple case that can be handled in this
> >>>patch.
> >>ups.
> >>>vpc is still easy, but a bit more complicated than a constant return 1,
> >>>because it depends on the subformat whether a new image will inherit the
> >>>has_zero_init property from the underlying storage or whether it always
> >>>produces zeros (for VHD_FIXED type images, it's basically raw + footer).
> >>>I'll send a separate patch for this.
> >>shall I leave this to the new 0 default until your patch is ready?
> >Yes, please leave vpc alone. I guess my patch will be queued before
> >yours anyway. ;-)
> once v2 is ready and noone has objections maybe you can put it in your queue? ;-)

Sure. I'm going to send a pull request today, so it would be good to
have v2 of this patch soon. (Otherwise it would have to wait for
Stefan's pull request probably next Friday)

Kevin

Re: [Qemu-devel] [PATCH] block: change default of .has_zero_init to 0

[Peter Lieven]

On 28.06.2013 12:09, Kevin Wolf wrote:
> Am 28.06.2013 um 10:25 hat Peter Lieven geschrieben:
>> On 28.06.2013 10:20, Kevin Wolf wrote:
>>> Am 28.06.2013 um 10:16 hat Peter Lieven geschrieben:
>>>> On 28.06.2013 10:06, Kevin Wolf wrote:
>>>>> Am 27.06.2013 um 15:52 hat Peter Lieven geschrieben:
>>>>>> .has_zero_init defaults to 1 for all formats and protocols.
>>>>>>
>>>>>> this is a dangerous default since this means that all
>>>>>> new added drivers need to manually overwrite it to 0 if
>>>>>> they do not ensure that a device is zero initialized
>>>>>> after bdrv_create().
>>>>>>
>>>>>> if a driver needs to explicitly set this value to
>>>>>> 1 its easier to verify the correctness in the review process.
>>>>>>
>>>>>> during review of the existing drivers it turned out
>>>>>> that ssh and gluster had a wrong default of 1.
>>>>>> both protocols support host_devices as backend
>>>>>> which are not by default zero initialized. this
>>>>>> wrong assumption will lead to possible corruption
>>>>>> if qemu-img convert is used to write to such a backend.
>>>>>>
>>>>>> a similar problem with the wrong default existed for
>>>>>> iscsi mose likely because the driver developer did
>>>>>> oversee the default value of 1.
>>>>>>
>>>>>> Signed-off-by: Peter Lieven <pl@kamp.de>
>>>>>> ---
>>>>>>   block.c               |    8 +++++++-
>>>>>>   block/qcow.c          |    1 +
>>>>>>   block/qcow2.c         |    1 +
>>>>>>   block/qed.c           |    1 +
>>>>>>   block/raw-posix.c     |   10 +---------
>>>>>>   block/raw-win32.c     |    7 +------
>>>>>>   block/rbd.c           |    1 +
>>>>>>   block/sheepdog.c      |    1 +
>>>>>>   block/vdi.c           |    1 +
>>>>>>   block/vmdk.c          |    1 +
>>>>>>   include/block/block.h |    1 +
>>>>>>   11 files changed, 17 insertions(+), 16 deletions(-)
>>>>> You forgot cow, which is also a simple case that can be handled in this
>>>>> patch.
>>>> ups.
>>>>> vpc is still easy, but a bit more complicated than a constant return 1,
>>>>> because it depends on the subformat whether a new image will inherit the
>>>>> has_zero_init property from the underlying storage or whether it always
>>>>> produces zeros (for VHD_FIXED type images, it's basically raw + footer).
>>>>> I'll send a separate patch for this.
>>>> shall I leave this to the new 0 default until your patch is ready?
>>> Yes, please leave vpc alone. I guess my patch will be queued before
>>> yours anyway. ;-)
>> once v2 is ready and noone has objections maybe you can put it in your queue? ;-)
> Sure. I'm going to send a pull request today, so it would be good to
> have v2 of this patch soon. (Otherwise it would have to wait for
> Stefan's pull request probably next Friday)
expect it in the next 60 minutes.

Peter

Re: [Qemu-devel] [PATCH] block: change default of .has_zero_init to 0

[Peter Lieven]

On 28.06.2013 10:06, Kevin Wolf wrote:
> Am 27.06.2013 um 15:52 hat Peter Lieven geschrieben:
>> .has_zero_init defaults to 1 for all formats and protocols.
>>
>> this is a dangerous default since this means that all
>> new added drivers need to manually overwrite it to 0 if
>> they do not ensure that a device is zero initialized
>> after bdrv_create().
>>
>> if a driver needs to explicitly set this value to
>> 1 its easier to verify the correctness in the review process.
>>
>> during review of the existing drivers it turned out
>> that ssh and gluster had a wrong default of 1.
>> both protocols support host_devices as backend
>> which are not by default zero initialized. this
>> wrong assumption will lead to possible corruption
>> if qemu-img convert is used to write to such a backend.
>>
>> a similar problem with the wrong default existed for
>> iscsi mose likely because the driver developer did
>> oversee the default value of 1.
>>
>> Signed-off-by: Peter Lieven <pl@kamp.de>
>> ---
>>   block.c               |    8 +++++++-
>>   block/qcow.c          |    1 +
>>   block/qcow2.c         |    1 +
>>   block/qed.c           |    1 +
>>   block/raw-posix.c     |   10 +---------
>>   block/raw-win32.c     |    7 +------
>>   block/rbd.c           |    1 +
>>   block/sheepdog.c      |    1 +
>>   block/vdi.c           |    1 +
>>   block/vmdk.c          |    1 +
>>   include/block/block.h |    1 +
>>   11 files changed, 17 insertions(+), 16 deletions(-)
> You forgot cow, which is also a simple case that can be handled in this
> patch.
Do we need to check the return value of bdrv_has_zero_init(bs->backing_hd)
in case bs-backing_hd != NULL?