Re: [Qemu-devel] [PATCH v5 05/10] block: Accept node-name arguments for block-commit

[Eric Blake]

On 06/13/2014 12:52 PM, Jeff Cody wrote:
> This modifies the block operation block-commit so that it will
> accept node-name arguments for either 'top' or 'base' BDS.
> 

>  void qmp_block_commit(const char *device,
>                        bool has_base, const char *base,
> +                      bool has_base_node_name, const char *base_node_name,
>                        bool has_top, const char *top,
> +                      bool has_top_node_name, const char *top_node_name,
>                        bool has_speed, int64_t speed,
>                        Error **errp)
>  {
> -    BlockDriverState *bs;
> -    BlockDriverState *base_bs, *top_bs;
> +    BlockDriverState *bs = NULL;
> +    BlockDriverState *base_bs = NULL;
> +    BlockDriverState *top_bs = NULL;
>      Error *local_err = NULL;
>      /* This will be part of the QMP command, if/when the
>       * BlockdevOnError change for blkmirror makes it in
> @@ -1940,20 +1943,33 @@ void qmp_block_commit(const char *device,
>      /* drain all i/o before commits */
>      bdrv_drain_all();
>  
> -    bs = bdrv_find(device);
> -    if (!bs) {
> -        error_set(errp, QERR_DEVICE_NOT_FOUND, device);

On IRC, we discussed that libvirt would like to rely on
QERR_DEVICE_NOT_FOUND (aka "class":"DeviceNotFound") as a witness probe
for whether qemu supports optional 'top' (which is a rough approximation
for whether qemu supports active commit, other than it mis-diagnoses
qemu 2.0; but libvirt probably wants to just require qemu 2.1 for active
commit anyways).  Compare:

on 2.1: {"execute":"block-commit","arguments":{"device":"no-such"}}
{"error": {"class": "DeviceNotFound", "desc": "Device 'no-such' not found"}}
on 1.7: {"execute":"block-commit","arguments":{"device":"no-such"}}
{"error": {"class": "GenericError", "desc": "Parameter 'top' is missing"}}

It might be worth a comment in the code that the device not found error
from a failed bdrv_find() should occur as early as possible, at least in
the case where all optional parameters are omitted, to serve as a
libvirt probe point (it is okay for the code to have various
GenericError sanity checks occur before the bdrv_find, so long as those
checks can only be triggered when optional parameters are present - but
again, worth mentioning in a comment added to the code).


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

signature.asc
Description: OpenPGP digital signature