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

[Stefan Hajnoczi]

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, ...