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

[Kevin Wolf]

Am 24.09.2013 um 07:18 hat Fam Zheng geschrieben:
> On Fri, 09/20 13:54, Kevin Wolf wrote:
> > For examples see the changes to qmp-commands.hx.
> > 
> > Signed-off-by: Kevin Wolf <address@hidden>
> > ---
> >  blockdev.c       |  57 ++++++++++++
> >  qapi-schema.json | 270 
> > +++++++++++++++++++++++++++++++++++++++++++++++++++++++
> >  qmp-commands.hx  |  59 ++++++++++++
> >  3 files changed, 386 insertions(+)
> > 
> > diff --git a/blockdev.c b/blockdev.c
> > index 977dc94..c4297d8 100644
> > --- a/blockdev.c
> > +++ b/blockdev.c
> > @@ -38,6 +38,8 @@
> >  #include "qemu/option.h"
> >  #include "qemu/config-file.h"
> >  #include "qapi/qmp/types.h"
> > +#include "qapi-visit.h"
> > +#include "qapi/qmp-output-visitor.h"
> >  #include "sysemu/sysemu.h"
> >  #include "block/block_int.h"
> >  #include "qmp-commands.h"
> > @@ -2066,6 +2068,61 @@ 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;
> > +    DriveInfo *dinfo;
> > +    Error *local_err = NULL;
> > +
> > +    /* Require an ID in the top level */
> > +    if (!options->has_id) {
> > +        error_setg(errp, "Block device needs an ID");
> > +        return;
> > +    }
> > +
> > +    /* TODO Sort it out in raw-posix and drive_init: Reject aio=native with
> > +     * cache.direct=false instead of silently switching to aio=threads, 
> > except
> > +     * if called from drive_init.
> > +     *
> > +     * For now, simply forbidding the combination for all drivers will do. 
> > */
> > +    if (options->has_aio && options->aio == BLOCKDEV_A_I_O_OPTIONS_NATIVE) 
> > {
> > +        bool direct = options->cache->has_direct && options->cache->direct;
> > +        if (!options->has_cache && !direct) {
> > +            error_setg(errp, "aio=native requires cache.direct=true");
> > +            goto fail;
> > +        }
> > +    }
> > +
> > +    visit_type_BlockdevOptions(qmp_output_get_visitor(ov),
> > +                               &options, NULL, &local_err);
> > +    if (error_is_set(&local_err)) {
> > +        error_propagate(errp, local_err);
> > +        goto fail;
> > +    }
> > +
> > +    obj = qmp_output_get_qobject(ov);
> > +    qdict = qobject_to_qdict(obj);
> > +
> > +    qdict_flatten(qdict);
> > +
> > +    QemuOpts *opts = qemu_opts_from_qdict(&qemu_drive_opts, qdict, 
> > &local_err);
> > +    if (error_is_set(&local_err)) {
> > +        error_propagate(errp, local_err);
> > +        goto fail;
> > +    } else {
> > +        dinfo = blockdev_init(opts, IF_NONE);
> > +        if (!dinfo) {
> > +            error_setg(errp, "Could not open image");
> > +            goto fail;
> > +        }
> > +    }
> > +
> > +fail:
> > +    qmp_output_visitor_cleanup(ov);
> > +}
> > +
> >  static void do_qmp_query_block_jobs_one(void *opaque, BlockDriverState *bs)
> >  {
> >      BlockJobInfoList **prev = opaque;
> > diff --git a/qapi-schema.json b/qapi-schema.json
> > index 145eca8..7e8ce60 100644
> > --- a/qapi-schema.json
> > +++ b/qapi-schema.json
> > @@ -3902,3 +3902,273 @@
> >  ##
> >  { '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.7
> > +##
> > +{ 'enum': 'BlockdevDiscardOptions',
> > +  'data': [ 'ignore', 'unmap' ] }
> > +
> > +##
> > +# @BlockdevAIOOptions
> > +#
> > +# Selects the AIO backend to handle I/O requests
> > +#
> > +# @threads:     Use qemu's thread pool
> > +# @native:      Use native AIO backend (only Linux and Windows)
> > +#
> > +# Since: 1.7
> > +##
> > +{ 'enum': 'BlockdevAIOOptions',
> > +  'data': [ 'threads', 'native' ] }
> > +
> > +##
> > +# @BlockdevCacheOptions
> > +#
> > +# Includes cache-related options for block devices
> > +#
> > +# @writeback:   #optional enables writeback mode for any caches (default: 
> > true)
> > +# @direct:      #optional enables use of O_DIRECT (bypass the host page 
> > cache;
> > +#               default: false)
> > +# @no-flush:    #optional ignore any flush requests for the device 
> > (default:
> > +#               false)
> > +#
> > +# Since: 1.7
> > +##
> > +{ 'type': 'BlockdevCacheOptions',
> > +  'data': { '*writeback': 'bool',
> > +            '*direct': 'bool',
> > +            '*no-flush': 'bool' } }
> > +
> > +##
> > +# @BlockdevThrottlingOptions
> > +#
> > +# Includes block device options related to I/O throttling. Leaving an 
> > option out
> > +# means the same as assigning 0 and applies no throttling.
> > +#
> > +# @bps-total:   #optional limit total bytes per second
> > +# @bps-read:    #optional limit read bytes per second
> > +# @bps-write:   #optional limit written bytes per second
> > +# @iops-total:  #optional limit total I/O operations per second
> > +# @iops-read:   #optional limit read operations per second
> > +# @iops-write:  #optional limit write operations per second
> > +#
> > +# Since: 1.7
> > +##
> > +{ 'type': 'BlockdevThrottlingOptions',
> > +  'data': { '*bps-total': 'int',
> > +            '*bps-read': 'int',
> > +            '*bps-write': 'int',
> > +            '*iops-total': 'int',
> > +            '*iops-read': 'int',
> > +            '*iops-write': 'int' } }
> > +
> > +##
> > +# @BlockdevOptionsBase
> > +#
> > +# Options that are available for all block devices, independent of the 
> > block
> > +# driver.
> > +#
> > +# @driver:      block driver name
> > +# @id:          #optional id by which the new block device can be referred 
> > to.
> > +#               This is a required option on the top level of 
> > blockdev-add, and
> > +#               currently not allowed on any other level.
> > +# @discard:     #optional discard-related options (default: ignore)
> > +# @cache:       #optional cache-related options
> > +# @aio:         #optional AIO backend (default: threads)
> > +# @rerror:      #optional how to handle read errors on the device
> > +#               (default: report)
> > +# @werror:      #optional how to handle write errors on the device
> > +#               (default: enospc)
> > +# @throttling:  #optional I/O throttling related options
> > +# @read-only:   #optional whether the block device should be read-only
> > +#               (default: false)
> > +#
> > +# Since: 1.7
> > +##
> > +{ 'type': 'BlockdevOptionsBase',
> > +  'data': { 'driver': 'str',
> > +            '*id': 'str',
> > +            '*discard': 'BlockdevDiscardOptions',
> > +            '*cache': 'BlockdevCacheOptions',
> > +            '*aio':  'BlockdevAIOOptions',
> > +            '*rerror': 'BlockdevOnError',
> > +            '*werror': 'BlockdevOnError',
> > +            '*throttling': 'BlockdevThrottlingOptions',
> > +            '*read-only': 'bool' } }
> > +
> > +##
> > +# @BlockdevOptionsFile
> > +#
> > +# Driver specific block device options for the file backend and similar
> > +# protocols.
> > +#
> > +# @filename:    path to the image file
> > +#
> > +# Since: 1.7
> > +##
> > +{ 'type': 'BlockdevOptionsFile',
> > +  'data': { 'filename': 'str' } }
> > +
> > +##
> > +# @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)
> > +#
> > +# Since: 1.7
> > +##
> > +{ 'type': 'BlockdevOptionsVVFAT',
> > +  'data': { 'dir': 'str', '*fat-type': 'int', '*floppy': 'bool',
> > +            '*rw': 'bool' } }
> > +
> > +##
> > +# @BlockdevOptionsGenericFormat
> > +#
> > +# Driver specific block device options for image format that have no option
> > +# besides their data source.
> > +#
> > +# @file:        reference to or definition of the data source block device
> > +#
> > +# Since: 1.7
> > +##
> > +{ '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:     #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' } }
> > +##
> > +# @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)
> > +#
> > +# @lazy-refcounts: #optional whether to enable the lazy refcounts feature
> > +#                  (default is taken from the image file)
> > +#
> > +# @pass-discard-request: #optional whether discard requests to the qcow2 
> > device
> > +#                        should be forwarded to the data source
> > +#
> > +# @pass-discard-snapshot: #optional 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: #optional whether discard requests for the data 
> > source
> > +#                      should be issued on other occasions where a cluster 
> > gets
> > +#                      freed
> > +#
> > +# Since: 1.7
> > +##
> > +{ 'type': 'BlockdevOptionsQcow2',
> > +  'base': 'BlockdevOptionsGenericCOWFormat',
> > +  'data': { '*lazy-refcounts': 'bool',
> > +            '*pass-discard-request': 'bool',
> > +            '*pass-discard-snapshot': 'bool',
> > +            '*pass-discard-other': 'bool' } }
> > +
> > +##
> > +# @BlockdevOptions
> > +#
> > +# Options for creating a block device.
> > +#
> > +# Since: 1.7
> > +##
> > +{ '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'?
> > +# TODO rbd: Wait for structured options
> > +# TODO sheepdog: Wait for structured options
> > +# TODO ssh: Should take InetSocketAddress for 'host'?
> > +      'vvfat':      'BlockdevOptionsVVFAT',
> > +
> > +# TODO blkdebug: Wait for structured options
> > +# TODO blkverify: Wait for structured options
> > +
> > +      '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'
> > +  } }
> > +
> > +##
> > +# @BlockdevRef
> > +#
> > +# Reference to a block device.
> > +#
> > +# @definition:      defines a new block device inline
> > +# @reference:       references the ID of an existing block device. An
> > +#                   empty string means that no block device should be
> > +#                   referenced.
> > +#
> > +# Since: 1.7
> > +##
> > +{ 'union': 'BlockdevRef',
> > +  'discriminator': {},
> > +  'data': { 'definition': 'BlockdevOptions',
> > +            'reference': 'str' } }
> > +
> > +##
> > +# @blockdev-add:
> > +#
> > +# Creates a new block device.
> > +#
> > +# @options: block device options for the new device
> > +#
> > +# Since: 1.7
> > +##
> > +{ 'command': 'blockdev-add', 'data': { 'options': 'BlockdevOptions' } }
> > diff --git a/qmp-commands.hx b/qmp-commands.hx
> > index b17c46e..449ecea 100644
> > --- a/qmp-commands.hx
> > +++ b/qmp-commands.hx
> > @@ -3240,3 +3240,62 @@ Example:
> >     }
> >  
> >  EQMP
> > +
> > +    {
> > +        .name       = "blockdev-add",
> > +        .args_type  = "options:q",
> > +        .mhandler.cmd_new = qmp_marshal_input_blockdev_add,
> > +    },
> > +
> > +SQMP
> > +blockdev-add
> > +------------
> > +
> > +Add a block device.
> > +
> > +Arguments:
> > +
> > +- "options": block driver options
> > +
> > +Example (1):
> > +
> > +-> { "execute": "blockdev-add",
> > +    "arguments": { "options" : { "driver": "qcow2",
> > +                                 "file": { "driver": "file",
> > +                                           "filename": "test.qcow2" } } } }
> > +<- { "return": {} }
> > +
> > +Example (2):
> > +
> > +-> { "execute": "blockdev-add",
> > +     "arguments": {
> > +         "options": {
> > +           "driver": "qcow2",
> > +           "id": "my_disk",
> > +           "discard": "unmap",
> > +           "throttling": {
> > +               "bps-total": 1234567,
> > +               "iops-write": 100
> > +           },
> > +           "cache": {
> > +               "direct": true,
> > +               "writeback": true
> > +           },
> > +           "file": {
> > +               "driver": "file",
> > +               "filename": "/tmp/test.qcow2"
> > +           },
> > +           "backing": {
> > +               "driver": "raw",
> > +               "file": {
> > +                   "driver": "file",
> > +                   "filename": "/dev/fdset/4"
> > +               }
> > +           }
> 
> How will BDS reference be used? Like this?
> 
>              "backing": "ide0-hd0",
> 
> [As I tested this is not supported with this series, the option "backing" is
> not parsed in bdrv_open(). Asking because image fleecing needs this, is it
> already in your todo list?]

Yes, this is what the syntax will be (and is already described in the
schema, even though it's not actually implemented yet).

It's in my todo list, but it won't come immediately after this series.
Leaving other todo items aside, one dependency that is missing for it is
that we make sure that having a named BDS in the middle of a tree
doesn't (allow users to) break things.

Kevin