Re: [Qemu-devel] [PATCH 2/3] qapi: Introduce blockdev-group-snapshot-sync command

[Eric Blake]

On 02/20/2012 10:31 AM, Jeff Cody wrote:
> This is a QAPI/QMP only command to take a snapshot of a group of
> devices. This is simlar to the blockdev-snapshot-sync command, except

s/simlar/similar/

> blockdev-group-snapshot-sync accepts a list devices, filenames, and
> formats.
> 
> It is attempted to keep the snapshot of the group atomic; if
> any snapshot of a device in a given group fails, then the whole group
> is reverted back to its original image, and error is reported.
> 
> This allows the group of disks to remain consistent with each other,
> even across snapshot failures.
> 
> Signed-off-by: Jeff Cody <address@hidden>
> ---
>  blockdev.c       |  130 
> ++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  qapi-schema.json |   45 +++++++++++++++++++
>  qmp-commands.hx  |   39 ++++++++++++++++
>  3 files changed, 214 insertions(+), 0 deletions(-)

> +
> +error_rollback:
> +    /* failure, undo everything as much as we can */
> +    QSIMPLEQ_FOREACH(snap_entry, &gsnp_list, entry) {
> +        if (snap_entry->has_pivoted) {
> +            ret = bdrv_open(snap_entry->bs, snap_entry->old_filename,
> +                            snap_entry->flags, snap_entry->old_drv);
> +            if (ret != 0) {
> +                /* This is very very bad */
> +                error_set(errp, QERR_OPEN_FILE_FAILED,
> +                          snap_entry->old_filename);

Is there any way to reduce the likelihood of a rollback failure?


> +SQMP
> +blockdev-group-snapshot-sync
> +----------------------
> +
> +Synchronous snapshot of one or more block devices.  A list array input
> +is accepted, that contains the device, snapshot-file to be create as the
> +target of the new image. If the file exists, or if it is a device, the
> +snapshot will be created in the existing file/device. If does not
> +exist, a new file will be created. format specifies the format of the
> +snapshot image, default is qcow2.  On failure of any device, it is
> +attempted to reopen the original image for all the devices that were
> +specified.
> +
> +Arguments:
> +
> +- "device": device name to snapshot (json-string)
> +- "snapshot-file": name of new image file (json-string)
> +- "format": format of new image (json-string, optional)

Shouldn't this mention that the arguments is a JSON list, rather than a
single argument?

> +
> +Example:
> +
> +-> { "execute": "blockdev-group-snapshot-sync", "arguments": [{ "device": 
> "ide-hd0",
> +                                                                
> "snapshot-file":
> +                                                                
> "/some/place/my-image",
> +                                                                "format": 
> "qcow2" },
> +                                                              { "device": 
> "ide-hd1",
> +                                                                
> "snapshot-file":
> +                                                                
> "/some/place/my-image2",
> +                                                                "format": 
> "qcow2" } }

Are you missing a ']' before the final '}' here?

-- 
Eric Blake   address@hidden    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

signature.asc
Description: OpenPGP digital signature