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

[Eric Blake]

On 07/23/2013 07:03 AM, Kevin Wolf wrote:

Is it worth a snippet of QMP wire code in the commit message, so that
someone reading 'git log' can more easily spot the impact of this patch?

> Signed-off-by: Kevin Wolf <address@hidden>
> ---
>  blockdev.c       |  45 +++++++++
>  qapi-schema.json | 293 
> +++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  qmp-commands.hx  |  26 +++++
>  3 files changed, 364 insertions(+)

> @@ -1805,6 +1807,49 @@ void qmp_block_job_complete(const char *device, Error 
> **errp)
>      block_job_complete(job, errp);
>  }
>  
> +void qmp_blockdev_add(BlockdevOptions *options, Error **errp)
> +{
> +    QmpOutputVisitor *ov = qmp_output_visitor_new();
> +    QObject *obj;
> +    QDict *qdict;
> +
> +    if (!options->has_id) {
> +        error_setg(errp, "Block device needs an ID");
> +        return;
> +    }

[1]

> +
> +    /* TODO Sort it out in raw-posix and drive_init: Reject aio=native with
> +     * cache.direct=off instead of silently switching to aio=threads, except 
> if

s/off/false/

> +     * called from drive_init.
> +     *
> +     * For now, simply forbidding the combination for all drivers will do. */

Does the TODO mean that more patches will be added later?  But I can
live with the limitation documented for this patch.

> +    if (options->has_aio && options->aio == BLOCKDEV_A_I_O_OPTIONS_NATIVE) {
> +        if (!options->has_cache || !options->cache->direct) {

options->cache->direct is listed as optional in the QAPI; does that mean
you need to first check options->cache->has_direct, or have you done a
sanity check elsewhere that provides sane defaults in the absence of
optional parameters?

> +++ b/qapi-schema.json
> @@ -3761,3 +3761,296 @@
>  ##
>  { 'command': 'query-rx-filter', 'data': { '*name': 'str' },
>    'returns': ['RxFilterInfo'] }
> +
> +
> +##
> +# @BlockdevDiscardOptions
> +#
> +# Determines how to handle discard requests.
> +#
> +# @ignore:      Ignore the request
> +# @unmap:       Forward as an unmap request
> +#
> +# Since: 1.6

Given the discussion earlier in the series, should this (and all other
new listings) mention 1.7?  Regarding your plan for committing framework
but then backing out the final implementation in time for 1.6, does that
mean applying the whole series or just the first 17 patches?  (Planning
for the timing of a commit doesn't necessarily invalidate the review,
but it's nice if we're all on the same page about when things will hit
qemu.git.)

> +{ 'type': 'BlockdevThrottlingOptions',
> +  'data': { '*bps-total': 'int',
> +            '*bps-read': 'int',
> +            '*bps-write': 'int',
> +            '*iops-total': 'int',
> +            '*iops-read': 'int',
> +            '*iops-write': 'int' } }

A lot of these types look like they would also be useful in output
structs; for example, do we need to modify BlockDeviceInfo?  But that
could be a separate patch.

> +
> +##
> +# @BlockdevOptionsBase
> +#
> +# Options that are available for all block devices, independent of the block
> +# driver.
> +#
> +# @driver:      block driver name
> +# @id:          id by which the new block device can be referred to
> +# @discard:     discard-related options
> +# @cache:       cache-related options
> +# @aio:         AIO backend (default: threads)
> +# @rerror:      how to handle read errors on the device (default: report)
> +# @werror:      how to handle write errors on the device (default: enospc)
> +# @throttling:  I/O throttling related options
> +# @read-only:   whether the block device should be read-only (default: false)
> +# @copy-on-read: whether to enable copy on read for the device (default: 
> false)

Is copy-on-read really applicable as a base option, or is it only
appropriate for COW formats such as qcow2 and qed?  Everything else
looks good, especially since you plan on allowing layering (for example,
a different rerror policy for a qcow2 primary image than what is used
for its backing file, regardless of whether the backing file is raw or
also qcow2).

Historically, we have marked optional members both with '*name' in the
actual qapi, but also with '#optional' in the text; is it worth another
pass over your patch to add in the missing '#optional' keywords?  [Since
nothing extracts docs automatically from the .json file yet, I'm okay if
such a pass is deferred to a followup patch]

> +#
> +# Since: 1.6
> +##
> +{ 'type': 'BlockdevOptionsBase',
> +  'data': { 'driver': 'str',
> +            '*id': 'str',

Per [1] above, you are failing if id was not provided.  Then why is it
marked optional?

> +{ 'type': 'BlockdevOptionsFile',
> +  'data': { 'filename': 'str' } }
> +
> +##
> +# @BlockdevOptionsFile

copy-and-paste; this should be BlockdevOptionsNBD

> +#
> +# Driver specific block device options for the NBD protocol. Either path or 
> host
> +# must be specified.
> +#
> +# @path:        path to a unix domain socket
> +# @host:        host name for a TCP connection
> +# @port:        port number for a TCP connection (default: 10809)
> +# @export:      NBD export name
> +#
> +# Since: 1.6
> +##
> +{ 'type': 'BlockdevOptionsNBD',
> +  'data': { '*path': 'str', '*host': 'str', '*port': 'int', '*export': 'str' 
> } }
> +
> +##
> +# @BlockdevOptionsSSH
> +#
> +# Driver specific block device options for the SSH protocol.
> +#
> +# @host:        host name for a TCP connection
> +# @port:        port number for a TCP connection (default: 22)
> +# @path:        path to the image on the remote host
> +# @user:        SSH user name
> +#
> +# @host-key-check:
> +# TODO Should this be split into multiple fields for QMP?
> +# TODO Driver takes host_key_check with underscores currently

We probably ought to polish that before actually committing to this QMP
interface.


> +# @BlockdevOptionsVVFAT
> +#
> +# Driver specific block device options for the vvfat protocol.
> +#
> +# @dir:         directory to be exported as FAT image
> +# @fat-type:    FAT type: 12, 16 or 32
> +# @floppy:      whether to export a floppy imae (true) or partitioned hard 
> disk
> +#               (false; default)
> +# @rw:          whether to allow write operations (default: false)

Does this duplicate the base class already providing 'read-only'?  And
if not, is there a nicer name than the abbreviation 'rw'?

> +{ 'type': 'BlockdevOptionsGenericFormat',
> +  'data': { 'file': 'BlockdevRef' } }
> +
> +##
> +# @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
> +# @backing:     reference to or definition of the backing file block device
> +#               (if missing, taken from the image file content)

Is it possible to request the empty string as @backing in order to force
the image to be used as though there were no backing file?  I'm not sure
if pulling such a stunt would ever make sense, but if we can override a
file with no stored content to use a backing file, it seems like there
should be a symmetrical way to override a file with a stored backing
file to be used without backing.

> +#
> +# Since: 1.6
> +##
> +# TODO Add inheritance and base on BlockdevOptionsGenericFormat

Yep, we discussed that on IRC, and I agreed that deferring complex
struct inheritance until later is not a show-stopper for this patch (but
WOULD make it easier to add a new field without having to touch multiple
types).

> +{ 'type': 'BlockdevOptionsGenericCOWFormat',
> +  'data': { 'file': 'BlockdevRef', '*backing': 'BlockdevRef' } }
> +
> +##
> +# @BlockdevOptionsGenericCOWFormat

Copy-and-paste, should be BlockdevOptionsQcow2

> +#
> +# Driver specific block device options for qcow2.
> +#
> +# @file:        reference to or definition of the data source block device
> +#
> +# @backing:     reference to or definition of the backing file block device
> +#               (if missing, taken from the image file content)
> +#
> +# @lazy-refcounts: whether to enable the lazy refcounts feature (default is
> +#                  taken from the image file)
> +#
> +# @pass-discard-request: whether discard requests to the qcow2 device should
> +#                        be forwarded to the data source
> +#
> +# @pass-discard-snapshot: whether discard requests for the data source should
> +#                         be issued when a snapshot operation (e.g. deleting
> +#                         a snapshot) frees clusters in the qcow2 file
> +#
> +# @pass-discard-other: whether discard requests for the data source should be
> +#                      issued on other occasions where a cluster gets freed

We also discussed this on IRC, and neither of us could come up with any
shorter names; anyone else want to take a stab at it?  But at least
adding docs helped compared to the scratch proposal you had run by me
via pastebin earlier :)

> +
> +##
> +# @BlockdevOptions
> +#
> +# Options for creating a block device.
> +#
> +# Since: 1.6
> +##
> +{ 'union': 'BlockdevOptions',
> +  'base': 'BlockdevOptionsBase',
> +  'discriminator': 'driver',
> +  'data': {
> +      'file':       'BlockdevOptionsFile',
> +      'http':       'BlockdevOptionsFile',
> +      'https':      'BlockdevOptionsFile',
> +      'ftp':        'BlockdevOptionsFile',
> +      'ftps':       'BlockdevOptionsFile',
> +      'tftp':       'BlockdevOptionsFile',
> +# TODO gluster: Wait for structured options
> +# TODO iscsi: Wait for structured options
> +# TODO nbd: Should take InetSocketAddress for 'host'?

You documented BlockdevOptionsNBD above, and then aren't using it here
because of the TODO.  It doesn't hurt the qapi code generator to
document unused types, but does look a bit fishy, especially since we
may want to change the contents of that type.  I guess it is things like
this which reinforce your desire to get framework in, but to disable the
feature until 1.7 has had more time to iron out warts before they become
permanent.

> +# TODO rbd: Wait for structured options
> +# TODO sheepdog: Wait for structured options
> +# TODO ssh: Should take InetSocketAddress for 'host'?
> +      'vvfat':      'BlockdevOptionsVVFAT',
> +
> +      'bochs':      'BlockdevOptionsGenericFormat',
> +      'cloop':      'BlockdevOptionsGenericFormat',
> +      'cow':        'BlockdevOptionsGenericCOWFormat',
> +      'dmg':        'BlockdevOptionsGenericFormat',
> +      'parallels':  'BlockdevOptionsGenericFormat',
> +      'qcow':       'BlockdevOptionsGenericCOWFormat',
> +      'qcow2':      'BlockdevOptionsQcow2',
> +      'qed':        'BlockdevOptionsGenericCOWFormat',
> +      'raw':        'BlockdevOptionsGenericFormat',
> +      'vdi':        'BlockdevOptionsGenericFormat',
> +      'vhdx':       'BlockdevOptionsGenericFormat',
> +      'vmdk':       'BlockdevOptionsGenericCOWFormat',
> +      'vpc':        'BlockdevOptionsGenericFormat'
> +  } }

If someone does go with the suggestion in 7/18 to make a discriminated
union key off an enum type instead of an open-coded string, then we
would also want to list the enum type that describes all the known
drivers for qemu.

> +
> +##
> +# @BlockdevRef
> +#
> +# Reference to a block device.
> +#
> +# @definition:      defines a new block device inline
> +# @reference:       references the ID of an existing block device
> +#
> +# Since: 1.6
> +##
> +{ 'union': 'BlockdevRef',
> +  'discriminator': {},
> +  'data': { 'definition': 'BlockdevOptions'

missing comma.  If Markus' re-write of the qapi parser gets applied
first, this wouldn't have happened :)

> +            'reference': 'str' } }
> +
> +##
> +# @blockdev-add:
> +#
> +# Creates a new block device.
> +#
> +# @options: block device options for the new device
> +#
> +# Since: 1.6
> +##
> +{ 'command': 'blockdev-add', 'data': { 'options': 'BlockdevOptions' } }

Looks deceptively simple, with all the complexity isolated into the type
definitions above.

> +Example:
> +
> +-> { "execute": "blockdev-add",
> +    "arguments": { "options" : { "driver": "qcow2",
> +                                 "file": { "driver": "file",
> +                                           "filename": "test.qcow2" } } } }
> +<- { "return": {} }

Is it worth more than one example, showing a bit more of the full power
of the schema?

Overall, I like where this is headed, but I'm not quite sure it is ready
for commit as-is; looking forward to v2.  Given my positive review on
the rest of the series, I think you could get away with a pull request
on the front half of the series and only respinning this patch, instead
of needing (re-)review of the entire series.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

signature.asc
Description: OpenPGP digital signature