Re: [Qemu-devel] [PATCH v2 08/12] qapi: Convert loadvm

[Kevin Wolf <kwolf@redhat.com>]

Am 24.04.2013 um 17:32 hat Pavel Hrdina geschrieben:
> QMP command vm-snapshot-load and HMP command loadvm behave similar to
> vm-snapshot-delete and delvm. The only different is that they will load
> the snapshot instead of deleting it.
> 
> Signed-off-by: Pavel Hrdina <phrdina@redhat.com>

>      bs = NULL;
>      while ((bs = bdrv_next(bs))) {
>          if (bdrv_can_snapshot(bs)) {
> -            bdrv_snapshot_goto(bs, name, &local_err);
> +            /* Small hack to ensure that proper snapshot is deleted for every
> +             * block driver. This will be fixed soon. */
> +            if (!strcmp(bs->drv->format_name, "rbd")) {
> +                bdrv_snapshot_goto(bs, sn.name, &local_err);
> +            } else {
> +                bdrv_snapshot_goto(bs, sn.id_str, &local_err);
> +            }

I think I wanted to comment this in an earlier patch in this series, but
didn't actually do it, so here is what I intended to write there:

"Umm... Just no."

Special casing an image format should _never_ happen. Even more so
outside block.c (and it's disliked even there). It's a sign that we're
doing something seriously wrong.

>              if (error_is_set(&local_err)) {
> -                qerror_report_err(local_err);
> +                error_setg(errp, "Failed to load snapshot for device '%s': %s",
> +                           bdrv_get_device_name(bs),
> +                           error_get_pretty(local_err));
>                  error_free(local_err);
> -                return -EIO;
> +                return NULL;
>              }
>          }
>      }

> --- a/vl.c
> +++ b/vl.c
> @@ -4376,8 +4376,13 @@ int main(int argc, char **argv, char **envp)
>  
>      qemu_system_reset(VMRESET_SILENT);
>      if (loadvm) {
> -        if (load_vmstate(loadvm) < 0) {
> +        Error *local_err = NULL;
> +        qmp_vm_snapshot_load(true, loadvm, false, NULL, &local_err);
> +
> +        if (error_is_set(&local_err)) {
>              autostart = 0;
> +            qerror_report_err(local_err);
> +            error_free(local_err);
>          }
>      }

Should we add something like "Loading the snapshot failed"? It's
probably not clear from all possible error messages.

Kevin