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
