Am 20.09.2013 um 17:22 hat Eric Blake geschrieben:
> On 09/20/2013 05:54 AM, Kevin Wolf wrote:
> > For examples see the changes to qmp-commands.hx.
> >
> > Signed-off-by: Kevin Wolf <kw...@redhat.com>
> > ---
> > blockdev.c | 57 ++++++++++++
> > qapi-schema.json | 270
> > +++++++++++++++++++++++++++++++++++++++++++++++++++++++
> > qmp-commands.hx | 59 ++++++++++++
> > 3 files changed, 386 insertions(+)
> >
>
>
> > +# Since: 1.7
> > +##
> > +{ 'type': 'BlockdevOptionsBase',
> > + 'data': { 'driver': 'str',
> > + '*id': 'str',
> > + '*discard': 'BlockdevDiscardOptions',
> > + '*cache': 'BlockdevCacheOptions',
> > + '*aio': 'BlockdevAIOOptions',
>
> Is the double space intentional? Harmless, but inconsistent.
No, I'll fix it.
> > + '*rerror': 'BlockdevOnError',
> > + '*werror': 'BlockdevOnError',
> > + '*throttling': 'BlockdevThrottlingOptions',
> > + '*read-only': 'bool' } }
> ...
>
> > +##
> > +# @BlockdevOptionsVVFAT
> > +#
> > +# Driver specific block device options for the vvfat protocol.
> > +#
> > +# @dir: directory to be exported as FAT image
> > +# @fat-type: #optional FAT type: 12, 16 or 32
> > +# @floppy: #optional whether to export a floppy imae (true) or
> > partitioned
> > +# hard disk (false; default)
> > +# @rw: #optional whether to allow write operations (default:
> > false)
>
> Why do we have 'read-only' in base, and 'rw' in vvfat? It feels like
> the vvfat option is redundant.
I guess it is kind of redundant. The reason for it is that it's always
been there, encoded in the filename as "fat:rw:..." - and the reason for
that is that read-write vvfat is even more unreliable than read-only
vvfat. So we have a read-only=false default in base, and a
read-only=true default in vvfat.
I'm not sure if changing this without breaking the command line
is possible; but if it is, it involves vvfat-specific magic in
drive_init(), which this simply isn't worth.
> > +##
> > +# @BlockdevOptionsGenericCOWFormat
> > +#
> > +# Driver specific block device options for image format that have no option
> > +# besides their data source and an optional backing file.
> > +#
> > +# @file: reference to or definition of the data source block device
>
> Do you need to document this field...
>
> > +# @backing: #optional reference to or definition of the backing file
> > block
> > +# device (if missing, taken from the image file content). It
> > is
> > +# allowed to pass an empty string here in order to disable
> > the
> > +# default backing file.
> > +# @copy-on-read: #optional whether to enable copy on read for the device
> > +# (default: false). Copy on read can only be used if the
> > +# image is not read-only.
> > +#
> > +# Since: 1.7
> > +##
> > +{ 'type': 'BlockdevOptionsGenericCOWFormat',
> > + 'base': 'BlockdevOptionsGenericFormat',
> > + 'data': { '*backing': 'BlockdevRef',
> > + '*copy-on-read': 'bool' } }
>
> ...given that it is only present by inheritence?
>
> > +##
> > +# @BlockdevOptionsQcow2
> > +#
> > +# Driver specific block device options for qcow2.
> > +#
> > +# @file: reference to or definition of the data source block device
> > +#
> > +# @backing: #optional reference to or definition of the backing file
> > block
> > +# device (if missing, taken from the image file content)
>
> Same question. If you DO document inherited fields, you missed
> @copy-on-read; if you DON'T document inherited fields, these two aren't
> necessary (instead, you could just have some statement about: "In
> addition to the fields documented in BlockdevOptionsGenericCOWFormat,
> this struct includes:")
Nope, I don't think they are necessary. I just wasn't careful enough
when I added the inheritance. I'll drop them.
Kevin
