Re: [Qemu-devel] [PULL 15/33] snapshot: distinguish id and name in snapshot delete

[Eric Blake <eblake@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 3143 bytes --]

On 09/13/2013 05:50 AM, Kevin Wolf wrote:
> From: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
> 
> Snapshot creation actually already distinguish id and name since it take
> a structured parameter *sn, but delete can't. Later an accurate delete
> is needed in qmp_transaction abort and blockdev-snapshot-delete-sync,
> so change its prototype. Also *errp is added to tip error, but return
> value is kepted to let caller check what kind of error happens. Existing

s/kepted/kept/

> caller for it are savevm, delvm and qemu-img, they are not impacted by
> introducing a new function bdrv_snapshot_delete_by_id_or_name(), which
> check the return value and do the operation again.
> 
> Before this patch:
>   For qcow2, it search id first then name to find the one to delete.
>   For rbd, it search name.
>   For sheepdog, it does nothing.
> 
> After this patch:
>   For qcow2, logic is the same by call it twice in caller.
>   For rbd, it always fails in delete with id, but still search for name
> in second try, no change to user.
> 
> Some code for *errp is based on Pavel's patch.
> 
> Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
> Signed-off-by: Pavel Hrdina <phrdina@redhat.com>
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---

>  
>  static int qemu_rbd_snap_remove(BlockDriverState *bs,
> -                                const char *snapshot_name)
> +                                const char *snapshot_id,
> +                                const char *snapshot_name,
> +                                Error **errp)
>  {
>      BDRVRBDState *s = bs->opaque;
>      int r;
>  
> +    if (!snapshot_name) {
> +        error_setg(errp, "rbd need a valid snapshot name");

s/need/needs/

> +        return -EINVAL;
> +    }
> +
> +    /* If snapshot_id is specified, it must be equal to name, see
> +       qemu_rbd_snap_list() */
> +    if (snapshot_id && strcmp(snapshot_id, snapshot_name)) {
> +        error_setg(errp,
> +                   "rbd do not support snapshot id, it should be NULL or "

s/do/does/

> -int bdrv_snapshot_delete(BlockDriverState *bs, const char *snapshot_id)
> +/**
> + * Delete an internal snapshot by @snapshot_id and @name.
> + * @bs: block device used in the operation
> + * @snapshot_id: unique snapshot ID, or NULL
> + * @name: snapshot name, or NULL
> + * @errp: location to store error
> + *
> + * If both @snapshot_id and @name are specified, delete the first one with
> + * id @snapshot_id and name @name.
> + * If only @snapshot_id is specified, delete the first one with id
> + * @snapshot_id.
> + * If only @name is specified, delete the first one with name @name.
> + * if none is specified, return -ENINVAL.

s/ENINVAL/EINVAL/

[Given that this is already in the pull request phase, I'm fine if the
typo in the commit message stays, and if the remaining typos are cleaned
up in a separate followup patch, rather than delaying this series any
longer - my fault for not reviewing sooner]

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 621 bytes --]