Re: [Qemu-devel] [PATCH 04/17] blockdev: 'blockdev-add' QMP command

[Kevin Wolf <kwolf@redhat.com>]

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 <kwolf@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