| Message ID | 1374584606-5615-19-git-send-email-kwolf@redhat.com |
|---|---|
| State | New |
| Headers | show |
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.
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
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?
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
δΊ 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 >
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
Signed-off-by: Kevin Wolf <kwolf@redhat.com> --- blockdev.c | 45 +++++++++ qapi-schema.json | 293 +++++++++++++++++++++++++++++++++++++++++++++++++++++++ qmp-commands.hx | 26 +++++ 3 files changed, 364 insertions(+)