Re: [Qemu-devel] [PATCH V3 4/9] qmp: add internal snapshot support in qmp_transaction

[Stefan Hajnoczi <stefanha@gmail.com>]

On Thu, Jun 27, 2013 at 10:41:43AM +0800, Wenchao Xia wrote:
> +    /* check whether a snapshot with name exist, no need to check id, since
> +       name will be checked later to make sure it does not mess up with id. */
> +    ret = bdrv_snapshot_find_by_id_and_name(bs, NULL, name, &sn, errp);
> +    if (error_is_set(errp)) {
> +        return;
> +    }
> +    if (ret) {
> +        error_setg(errp,
> +                   "Snapshot with name '%s' already exist on device '%s'",

s/exist/exists/

> +                   name, device);
> +        return;
> +    }
> +
> +    /* Forbid having a name similar to id, empty name is also forbidden. */
> +    if (!snapshot_name_wellformed(name)) {
> +        error_setg(errp, "Name '%s' on device '%s' is not a valid one",
> +                   name, device);
> +        return;
> +    }
> +
> +    /* 3. take the snapshot */
> +    sn1 = &state->sn;
> +    pstrcpy(sn1->name, sizeof(sn1->name), name);
> +    qemu_gettimeofday(&tv);
> +    sn1->date_sec = tv.tv_sec;
> +    sn1->date_nsec = tv.tv_usec * 1000;
> +    sn1->vm_clock_nsec = qemu_get_clock_ns(vm_clock);
> +
> +    if (bdrv_snapshot_create(bs, sn1) < 0) {
> +        error_setg(errp, "Failed to create snapshot '%s' on device '%s'",
> +                   name, device);

Please use error_setg_errno() to include the bdrv_snapshot_create()
error message.

> @@ -1009,6 +1010,18 @@ the new image file has the same contents as the current one; QEMU cannot
>  perform any meaningful check.  Typically this is achieved by using the
>  current image file as the backing file for the new image.
>  
> +On failure, the original disks pre-snapshot attempt will be used.
> +
> +For internal snapshots, the dictionary contains the device and the snapshot's
> +name.  If name is a numeric string which will mess up with ID, the request will

This is about namespace collision.  "Collide" or "conflict" are usually
used to describe identical naming problems.  Instead of "mess up" I
would say something like:

The name must not be a numeric string since this collides with snapshot
IDs and an error will be returned.

> +be rejected.  For example, name "99" is not a valid name.  If an internal
> +snapshot matching name already exists, the request will be also rejected.  Only
> +some image formats support it, for example, qcow2, rbd, and sheepdog.
> +
> +On failure, qemu will try delete new created internal snapshot in the

s/new created/the newly created/

> +transaction.  When I/O error causes deletion failure, the user needs to fix it

When an I/O error occurs during deletion, ...