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

[Kevin Wolf <kwolf@redhat.com>]

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

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 <kwolf@redhat.com>
> > ---
> >  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).

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.

> 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.

Kevin

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]