Patchwork [18/18] blockdev: 'blockdev-add' QMP command

login
register
mail settings
Submitter Kevin Wolf
Date July 23, 2013, 1:03 p.m.
Message ID <1374584606-5615-19-git-send-email-kwolf@redhat.com>
Download mbox | patch
Permalink /patch/261080/
State New
Headers show

Comments

Kevin Wolf - July 23, 2013, 1:03 p.m.
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 blockdev.c       |  45 +++++++++
 qapi-schema.json | 293 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
 qmp-commands.hx  |  26 +++++
 3 files changed, 364 insertions(+)
Eric Blake - July 26, 2013, 5:45 p.m.
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 <kwolf@redhat.com>
> ---
>  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.
Kevin Wolf - July 26, 2013, 6:14 p.m.
Am 26.07.2013 um 19:45 hat Eric Blake geschrieben:
> 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.

Yup, that's exactly what I'm going to do now. I had considered merging
the whole series, but I think you're right that committing it now might
be a bit early. My main concern were potential conflicts in the front
"half" (17/18) of the series when I'd wait too long with committing.

I'll reply in more detail to your other points later today or on Monday.

Kevin
Luiz Capitulino - July 30, 2013, 5:44 p.m.
On Fri, 26 Jul 2013 20:14:06 +0200
Kevin Wolf <kwolf@redhat.com> wrote:

> Am 26.07.2013 um 19:45 hat Eric Blake geschrieben:
> > 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.
> 
> Yup, that's exactly what I'm going to do now. I had considered merging
> the whole series, but I think you're right that committing it now might
> be a bit early. My main concern were potential conflicts in the front
> "half" (17/18) of the series when I'd wait too long with committing.

Just to double check I got it as I'm still catching up with email
after vacation: you did exactly what was suggested by Eric, you
merged 1-17 and will post the blockdev-add command for 1.7?
Kevin Wolf - July 31, 2013, 8:09 a.m.
Am 30.07.2013 um 19:44 hat Luiz Capitulino geschrieben:
> On Fri, 26 Jul 2013 20:14:06 +0200
> Kevin Wolf <kwolf@redhat.com> wrote:
> 
> > Am 26.07.2013 um 19:45 hat Eric Blake geschrieben:
> > > 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.
> > 
> > Yup, that's exactly what I'm going to do now. I had considered merging
> > the whole series, but I think you're right that committing it now might
> > be a bit early. My main concern were potential conflicts in the front
> > "half" (17/18) of the series when I'd wait too long with committing.
> 
> Just to double check I got it as I'm still catching up with email
> after vacation: you did exactly what was suggested by Eric, you
> merged 1-17 and will post the blockdev-add command for 1.7?

Correct. Patches 1-17 are in master now, and blockdev-add itself will
be reposted during the 1.7 cycle.

Kevin
Wayne Xia - Aug. 30, 2013, 7:41 a.m.
于 2013-7-23 21:03, Kevin Wolf 写道:
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>   blockdev.c       |  45 +++++++++
>   qapi-schema.json | 293 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
>   qmp-commands.hx  |  26 +++++
>   3 files changed, 364 insertions(+)
> 
> diff --git a/blockdev.c b/blockdev.c
> index ef55b1a..214173b 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"
> @@ -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;
> +    }
> +
> +    /* 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
> +     * 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) {
> +        if (!options->has_cache || !options->cache->direct) {
> +            error_setg(errp, "aio=native requires cache.direct=true");
> +            return;
> +        }
> +    }
> +
> +    visit_type_BlockdevOptions(qmp_output_get_visitor(ov),
> +                               &options, NULL, errp);
> +    obj = qmp_output_get_qobject(ov);
> +    qdict = qobject_to_qdict(obj);
> +
> +    qdict_flatten(qdict);
> +
> +    Error *local_err = NULL;
> +    QemuOpts *opts = qemu_opts_from_qdict(&qemu_drive_opts, qdict, &local_err);
> +    if (error_is_set(&local_err)) {
> +        qerror_report_err(local_err);
> +        error_free(local_err);
> +    } else {
> +        blockdev_init(opts, IF_NONE);
> +    }
> +
> +    qobject_decref(obj);
> +    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 592bb9c..bfe6d32 100644
> --- a/qapi-schema.json
> +++ 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
> +##
> +{ 'enum': 'BlockdevDiscardOptions',
> +  'data': [ 'ignore', 'unmap' ] }
> +
> +##
> +# @BlockdevDiscardOptions
> +#
> +# 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.6
> +##
> +{ 'enum': 'BlockdevAIOOptions',
> +  'data': [ 'threads', 'native' ] }
> +
> +##
> +# @BlockdevCacheOptions
> +#
> +# Includes cache-related options for block devices
> +#
> +# @writeback:   enables writeback mode for any caches (default: true)
> +# @direct:      enables use of O_DIRECT (bypass the host page cache;
> +#               default: false)
> +# @no-flush:    ignore any flush requests for the device (default: false)
> +#
> +# Since: 1.6
> +##
> +{ '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:   limit total bytes per second
> +# @bps-read:    limit read bytes per second
> +# @bps-write:   limit written bytes per second
> +# @iops-total:  limit total I/O operations per second
> +# @iops-read:   limit read operations per second
> +# @iops-write:  limit write operations per second
> +#
> +# Since: 1.6
> +##
> +{ '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:          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)
> +#
> +# Since: 1.6
> +##
> +{ 'type': 'BlockdevOptionsBase',
> +  'data': { 'driver': 'str',
> +            '*id': 'str',
> +            '*discard': 'BlockdevDiscardOptions',
> +            '*cache': 'BlockdevCacheOptions',
> +            '*aio':  'BlockdevAIOOptions',
> +            '*rerror': 'BlockdevOnError',
> +            '*werror': 'BlockdevOnError',
> +            '*throttling': 'BlockdevThrottlingOptions',
> +            '*read-only': 'bool',
> +            '*copy-on-read': 'bool' } }
> +
> +##
> +# @BlockdevOptionsFile
> +#
> +# Driver specific block device options for the file backend and similar
> +# protocols.
> +#
> +# @filename:    path to the image file
> +#
> +# Since: 1.6
> +##
> +{ 'type': 'BlockdevOptionsFile',
> +  'data': { 'filename': 'str' } }
> +
> +##
> +# @BlockdevOptionsFile
> +#
> +# 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
> +#
> +# Since: 1.6
> +##
> +{ 'type': 'BlockdevOptionsSSH',
> +  'data': { 'host': 'str', '*port': 'int', 'path': 'str', '*user': 'str',
> +            '*host-key-check': 'str' } }
> +
> +##
> +# @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)
> +#
> +# Since: 1.6
> +##
> +{ 'type': 'BlockdevOptionsVVFAT',
> +  'data': { 'dir': 'str', '*fat-type': 'int', '*floppy': 'bool',
> +            '*rw': 'bool' } }
> +

  Paid some time to find out the organization of options, I think well
named type can help user understand better, how about rename as:
BlockdevOptionsFile --> BlockdevOptionsProtocolFile
BlockdevOptionsNBD --> BlockdevOptionsProtocolNBD
BlockdevOptionsSSH --> BlockdevOptionsProtocolSSH
BlockdevOptionsVVFAT --> BlockdevOptionsProtocolVVFAT

BlockdevOptionsGenericFormat --> BlockdevOptionsFormatGeneric
BlockdevOptionsGenericCOWFormat -->BlockdevOptionsFormatCowGeneric
....

> +##
> +# @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.6
> +##
> +{ '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)
> +#
> +# Since: 1.6
> +##
> +# TODO Add inheritance and base on BlockdevOptionsGenericFormat
> +{ 'type': 'BlockdevOptionsGenericCOWFormat',
> +  'data': { 'file': 'BlockdevRef', '*backing': 'BlockdevRef' } }
> +
> +##
> +# @BlockdevOptionsGenericCOWFormat
> +#
> +# 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
> +#
> +# Since: 1.6
> +##
> +# TODO Add inheritance and base on BlockdevOptionsGenericCOWFormat
> +{ 'type': 'BlockdevOptionsQcow2',
> +  'data': { 'file': 'BlockdevRef',
> +            '*backing': 'BlockdevRef',
> +            '*lazy-refcounts': 'bool',
> +            '*pass-discard-request': 'bool',
> +            '*pass-discard-snapshot': 'bool',
> +            '*pass-discard-other': 'bool' } }
> +
> +##
> +# @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'?
> +# 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'
> +  } }
> +
> +##
> +# @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'
> +            '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' } }
> diff --git a/qmp-commands.hx b/qmp-commands.hx
> index 65a9e26..96b0956 100644
> --- a/qmp-commands.hx
> +++ b/qmp-commands.hx
> @@ -3113,3 +3113,29 @@ 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:
> +
> +-> { "execute": "blockdev-add",
> +    "arguments": { "options" : { "driver": "qcow2",
> +                                 "file": { "driver": "file",
> +                                           "filename": "test.qcow2" } } } }
> +<- { "return": {} }
> +
> +EQMP
>

Patch

diff --git a/blockdev.c b/blockdev.c
index ef55b1a..214173b 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"
@@ -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;
+    }
+
+    /* 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
+     * 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) {
+        if (!options->has_cache || !options->cache->direct) {
+            error_setg(errp, "aio=native requires cache.direct=true");
+            return;
+        }
+    }
+
+    visit_type_BlockdevOptions(qmp_output_get_visitor(ov),
+                               &options, NULL, errp);
+    obj = qmp_output_get_qobject(ov);
+    qdict = qobject_to_qdict(obj);
+
+    qdict_flatten(qdict);
+
+    Error *local_err = NULL;
+    QemuOpts *opts = qemu_opts_from_qdict(&qemu_drive_opts, qdict, &local_err);
+    if (error_is_set(&local_err)) {
+        qerror_report_err(local_err);
+        error_free(local_err);
+    } else {
+        blockdev_init(opts, IF_NONE);
+    }
+
+    qobject_decref(obj);
+    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 592bb9c..bfe6d32 100644
--- a/qapi-schema.json
+++ 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
+##
+{ 'enum': 'BlockdevDiscardOptions',
+  'data': [ 'ignore', 'unmap' ] }
+
+##
+# @BlockdevDiscardOptions
+#
+# 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.6
+##
+{ 'enum': 'BlockdevAIOOptions',
+  'data': [ 'threads', 'native' ] }
+
+##
+# @BlockdevCacheOptions
+#
+# Includes cache-related options for block devices
+#
+# @writeback:   enables writeback mode for any caches (default: true)
+# @direct:      enables use of O_DIRECT (bypass the host page cache;
+#               default: false)
+# @no-flush:    ignore any flush requests for the device (default: false)
+#
+# Since: 1.6
+##
+{ '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:   limit total bytes per second
+# @bps-read:    limit read bytes per second
+# @bps-write:   limit written bytes per second
+# @iops-total:  limit total I/O operations per second
+# @iops-read:   limit read operations per second
+# @iops-write:  limit write operations per second
+#
+# Since: 1.6
+##
+{ '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:          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)
+#
+# Since: 1.6
+##
+{ 'type': 'BlockdevOptionsBase',
+  'data': { 'driver': 'str',
+            '*id': 'str',
+            '*discard': 'BlockdevDiscardOptions',
+            '*cache': 'BlockdevCacheOptions',
+            '*aio':  'BlockdevAIOOptions',
+            '*rerror': 'BlockdevOnError',
+            '*werror': 'BlockdevOnError',
+            '*throttling': 'BlockdevThrottlingOptions',
+            '*read-only': 'bool',
+            '*copy-on-read': 'bool' } }
+
+##
+# @BlockdevOptionsFile
+#
+# Driver specific block device options for the file backend and similar
+# protocols.
+#
+# @filename:    path to the image file
+#
+# Since: 1.6
+##
+{ 'type': 'BlockdevOptionsFile',
+  'data': { 'filename': 'str' } }
+
+##
+# @BlockdevOptionsFile
+#
+# 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
+#
+# Since: 1.6
+##
+{ 'type': 'BlockdevOptionsSSH',
+  'data': { 'host': 'str', '*port': 'int', 'path': 'str', '*user': 'str',
+            '*host-key-check': 'str' } }
+
+##
+# @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)
+#
+# Since: 1.6
+##
+{ '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.6
+##
+{ '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)
+#
+# Since: 1.6
+##
+# TODO Add inheritance and base on BlockdevOptionsGenericFormat
+{ 'type': 'BlockdevOptionsGenericCOWFormat',
+  'data': { 'file': 'BlockdevRef', '*backing': 'BlockdevRef' } }
+
+##
+# @BlockdevOptionsGenericCOWFormat
+#
+# 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
+#
+# Since: 1.6
+##
+# TODO Add inheritance and base on BlockdevOptionsGenericCOWFormat
+{ 'type': 'BlockdevOptionsQcow2',
+  'data': { 'file': 'BlockdevRef',
+            '*backing': 'BlockdevRef',
+            '*lazy-refcounts': 'bool',
+            '*pass-discard-request': 'bool',
+            '*pass-discard-snapshot': 'bool',
+            '*pass-discard-other': 'bool' } }
+
+##
+# @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'?
+# 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'
+  } }
+
+##
+# @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'
+            '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' } }
diff --git a/qmp-commands.hx b/qmp-commands.hx
index 65a9e26..96b0956 100644
--- a/qmp-commands.hx
+++ b/qmp-commands.hx
@@ -3113,3 +3113,29 @@  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:
+
+-> { "execute": "blockdev-add",
+    "arguments": { "options" : { "driver": "qcow2",
+                                 "file": { "driver": "file",
+                                           "filename": "test.qcow2" } } } }
+<- { "return": {} }
+
+EQMP