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

[Peter Lieven <pl@kamp.de>]

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