Re: [Qemu-block] [RFC PATCH 02/10] block/qapi: Add qcow2 create options to schema

[Eric Blake]

On 01/16/2018 02:11 PM, Kevin Wolf wrote:

>>> +{ 'enum': 'BlockdevQcow2CompatLevel',
>>> +  'data': [ '0_10', '1_1' ] }
>>
>> Enums are allowed to start with digits while struct members are not; so
>> you can get away with this naming.  Do we really want the names 0_10 and
>> 1_1, or are there better names we could come up with (it already
>> undergoes translation such that qemu-img reports 0.10 rather than 0_10).
> 
> Yeah, I don't like 0_10/1_1 much.
> 
> Either we allow dots in enum values so that we can keep 0.10/1.1, or
> something completely different. I was considering 'version': 'int' with
> 2 and 3 as possible values, after all QMP is already rather low-level.
> 

I can live with a lower-level 'version':'int' for qcow2 creation over QMP.

> The question is just what to do with the command line. Will we deprecate
> compat=0.10/1.1 there, too, and tell users to switch to whatever new
> syntax we invent for QMP? Or are we planning to keep the "translation"
> from the old syntax forever?

At a minimum, we'll have to keep the translation syntax for as long as a
deprecation cycle with proper documentation is available (at least two
releases); keeping it longer than that depends on whether we think the
deprecation is worth the cleaner code in the long run.  But we do have a
deprecation policy, so we can start thinking about using that now so
that in another year we can do a release that gets rid of the
back-compat code.


>>> +  'data': { 'size':             'size',
>>
>> Is size mandatory even when we have a backing file specification?  It is
>> not mandatory for qemu-img create; but on the other hand, I think I can
>> live with requiring the QMP caller to supply a size.
> 
> The qemu-img create implementation of this is common code at least, but
> we're in driver-specific definitions here, so every driver would have to
> call some function to guess the size given a backing file string. With
> the straightforward implementation of this series, it is really
> mandatory because otherwise you'd get zero-sized images.
> 
> Accessing the backing file during image creation is also one of those
> things that tend to cause surprises, so if we don't have to, I wouldn't
> do that.

Good point.  So mandatory size at the QMP layer makes sense (qemu-img
can still open multiple images to determine what size to pass to QMP
under the hood).

> 
>>> +            '*compat':          'BlockdevQcow2CompatLevel',
>>> +            '*backing-file':    'str',
>>
>> Given Dan's comments, perhaps name this one 'backing-str' to make it
>> obvious that it is the string written into the qcow2 header, rather than
>> the node we open as backing?
> 
> If you guys think that this is clearer, I can change it.

Especially since you're convincing me that we DON'T want to open a
backing node during this operation, I think backing-str is a bit clearer
(of course, that's another place for command-line back-compat glue that
we may want to deprecate over time).

> 
>> Or, maybe we support an optional '*backing-node' that can be used for
>> allowing a default size and backing string if not explicitly
>> overridden?
> 
> Hm, it would make the interface a bit more complex. I'd try whether we
> can do without it.

I'm fine if you can manage the series without having a backing-node
argument.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

signature.asc
Description: OpenPGP digital signature