Re: [Qemu-devel] [PATCH 05/10] snapshot: create bdrv_all_find_snapshot helper

[Stefan Hajnoczi <stefanha@gmail.com>]

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

On Tue, Nov 10, 2015 at 05:25:30PM +0300, Denis V. Lunev wrote:
> +int bdrv_all_find_snapshot(const char *name, bool read_only,
> +                           BlockDriverState **first_bad_bs)
> +{
> +    QEMUSnapshotInfo sn;
> +    int err = 0;
> +    BlockDriverState *bs = NULL;
> +
> +    while (err == 0 && (bs = bdrv_next(bs))) {
> +        AioContext *ctx = bdrv_get_aio_context(bs);
> +
> +        aio_context_acquire(ctx);
> +        if (read_only || (bdrv_is_inserted(bs) && !bdrv_is_read_only(bs))) {
> +            err = bdrv_snapshot_find(bs, &sn, name);
> +        }
> +        aio_context_release(ctx);
> +    }
> +
> +    *first_bad_bs = bs;
> +    return err;
> +}

It's difficult to see how bdrv_all_find_snapshot(read_only=true) is
equivalent to what you replaced below:

> @@ -1500,21 +1489,7 @@ void hmp_info_snapshots(Monitor *mon, const QDict *qdict)
>      available_snapshots = g_new0(int, nb_sns);
>      total = 0;
>      for (i = 0; i < nb_sns; i++) {
> -        sn = &sn_tab[i];
> -        available = 1;
> -        bs1 = NULL;
> -
> -        while ((bs1 = bdrv_next(bs1))) {
> -            if (bdrv_can_snapshot(bs1) && bs1 != bs) {
> -                ret = bdrv_snapshot_find(bs1, sn_info, sn->id_str);
> -                if (ret < 0) {
> -                    available = 0;
> -                    break;
> -                }
> -            }
> -        }
> -
> -        if (available) {
> +        if (bdrv_all_find_snapshot(sn_tab[i].id_str, true, &bs1) == 0) {

The original loop skips drives that cannot snapshot and the vmstate
drive.  The new code tries to find a snapshot all devices.

To me it seems the semantics are changed.  Can you explain why this
change is safe?

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]