Re: [Qemu-devel] [PATCH 7/7] vpc: Support .bdrv_co_create

[Max Reitz]

On 2018-03-13 12:32, Kevin Wolf wrote:
> Am 12.03.2018 um 22:49 hat Max Reitz geschrieben:
>> On 2018-03-09 22:46, Kevin Wolf wrote:
>>> This adds the .bdrv_co_create driver callback to vpc, which
>>> enables image creation over QMP.
>>>
>>> Signed-off-by: Kevin Wolf <address@hidden>
>>> ---
>>>  qapi/block-core.json |  33 ++++++++++-
>>>  block/vpc.c          | 152 
>>> ++++++++++++++++++++++++++++++++++++++-------------
>>>  2 files changed, 147 insertions(+), 38 deletions(-)
>>>
>>> diff --git a/qapi/block-core.json b/qapi/block-core.json
>>> index 3a65909c47..ca645a0067 100644
>>> --- a/qapi/block-core.json
>>> +++ b/qapi/block-core.json
>>> @@ -3734,6 +3734,37 @@
>>>              '*block-state-zero':    'bool' } }
>>>  
>>>  ##
>>> +# @BlockdevVpcSubformat:
>>> +#
>>> +# @dynamic: Growing image file
>>> +# @fixed:   Preallocated fixed-size imge file
>>
>> s/imge/image/
>>
>>> +#
>>> +# Since: 2.12
>>> +##
>>> +{ 'enum': 'BlockdevVpcSubformat',
>>> +  'data': [ 'dynamic', 'fixed' ] }
>>> +
>>> +##
>>> +# @BlockdevCreateOptionsVpc:
>>> +#
>>> +# Driver specific image creation options for vpc (VHD).
>>> +#
>>> +# @file             Node to create the image format on
>>> +# @size             Size of the virtual disk in bytes
>>> +# @subformat        vhdx subformat (default: dynamic)
>>> +# @force-size       Force use of the exact byte size instead of rounding 
>>> to the
>>> +#                   next size that can be represented in CHS geometry
>>> +#                   (default: false)
>>
>> Now that's weird, again considering your previous approach of only
>> rounding things in the legacy path and instead throwing errors from
>> blockdev-create.  If you think this is OK to have here, than that's OK
>> with me, but I'm not sure this is the ideal way.
> 
> Hmm... That's a tough one.
> 
> There are a two major differences between VHD and the other image
> formats: The first is that rounding is part of the VHD spec. The other
> is that while other drivers have reasonable alignment restrictions that
> never cause a problem anyway (because people say just '8G' instead of
> some odd number), CHS alignment is not reasonable (because '8G' and
> similar things will most probably fail).

Well, if it's part of the spec, then I'll be OK with keeping the flag as
it is.

> And then there's the well-known problem that MS is inconsistent with
> itself, so force-size=off is required to make images that work with
> Virtual PC, but force-size=on may be needed for unaligned image sizes
> that HyperV allows, iirc.
> 
>> Alternatives:
>>
>> 1. Swap the default, not sure this is such a good idea either.
>>
>> 2. Maybe add an enum instead.  Default: If the given size doesn't fit
>> CHS, generate an error.  Second choice: Use the given size, even if it
>> doesn't fit.  Third choice: Round to CHS.
> 
> Maybe we should keep force-size, but make it an error if the size isn't
> already aligned (consistent with other block drivers).
> 
> The legacy code path could still round, but print a deprecation warning.
> Once we get rid of the legacy path, users will have to specify sizes
> with correct alignment. The error message could suggest using the
> rounded value for Virtual PC compatibility or force-share=on otherwise.
> 
> That wouldn't be very nice to use, but maybe it's the best we can make
> out of a messed up format like VHD.

Sounds reasonable to me, although this would probably just result in
management tools copying qemu's code (or maybe it's code directly from
the spec?) to do the rounding so that qemu shuts up.

>> I don't want to be stuck up, but once it's a public interface...
> 
> The good thing is that it's still x-blockdev-create.

Ah, right.

Max

signature.asc
Description: OpenPGP digital signature