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 <kw...@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. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
signature.asc
Description: OpenPGP digital signature