Re: [Qemu-devel] [PATCH v2 05/12] block: update error reporting for bdrv_snapshot_goto() and related functions

[Kevin Wolf]

Am 24.04.2013 um 17:32 hat Pavel Hrdina geschrieben:
> Signed-off-by: Pavel Hrdina <address@hidden>

> --- a/block/sheepdog.c
> +++ b/block/sheepdog.c
> @@ -1867,7 +1867,9 @@ cleanup:
>      return ret;
>  }
>  
> -static int sd_snapshot_goto(BlockDriverState *bs, const char *snapshot_id)
> +static void sd_snapshot_goto(BlockDriverState *bs,
> +                             const char *snapshot_id,
> +                             Error **errp)
>  {
>      BDRVSheepdogState *s = bs->opaque;
>      BDRVSheepdogState *old_s;
> @@ -1892,13 +1894,13 @@ static int sd_snapshot_goto(BlockDriverState *bs, 
> const char *snapshot_id)
>  
>      ret = find_vdi_name(s, vdi, snapid, tag, &vid, 1);
>      if (ret) {
> -        error_report("Failed to find_vdi_name");
> +        error_setg_errno(errp, -ret, "Failed to find VDI snapshot '%s'", 
> vdi);

Isn't snapid what identifies the snapshot? Or maybe the combination of
vdi and snapid.

>          goto out;
>      }
>  
>      fd = connect_to_sdog(s);
>      if (fd < 0) {
> -        ret = fd;
> +        error_setg_errno(errp, -fd, "Failed to connect to sdog");
>          goto out;
>      }
>  
> @@ -1909,14 +1911,14 @@ static int sd_snapshot_goto(BlockDriverState *bs, 
> const char *snapshot_id)
>      closesocket(fd);
>  
>      if (ret) {
> +        error_setg_errno(errp, -ret, "Failed to open VDI snapshot '%s'", 
> vdi);
>          goto out;
>      }
>  
>      memcpy(&s->inode, buf, sizeof(s->inode));
>  
>      if (!s->inode.vm_state_size) {
> -        error_report("Invalid snapshot");
> -        ret = -ENOENT;
> +        error_setg(errp, "Invalid snapshot '%s'", snapshot_id);

Here as well.

>          goto out;
>      }
>  
> @@ -1925,16 +1927,12 @@ static int sd_snapshot_goto(BlockDriverState *bs, 
> const char *snapshot_id)
>      g_free(buf);
>      g_free(old_s);
>  
> -    return 0;
> +    return;
>  out:
>      /* recover bdrv_sd_state */
>      memcpy(s, old_s, sizeof(BDRVSheepdogState));
>      g_free(buf);
>      g_free(old_s);
> -
> -    error_report("failed to open. recover old bdrv_sd_state.");
> -
> -    return ret;
>  }
>  
>  static void sd_snapshot_delete(BlockDriverState *bs,

> --- a/savevm.c
> +++ b/savevm.c
> @@ -2529,11 +2529,11 @@ int load_vmstate(const char *name)
>      bs = NULL;
>      while ((bs = bdrv_next(bs))) {
>          if (bdrv_can_snapshot(bs)) {
> -            ret = bdrv_snapshot_goto(bs, name);
> -            if (ret < 0) {
> -                error_report("Error %d while activating snapshot '%s' on 
> '%s'",
> -                             ret, name, bdrv_get_device_name(bs));
> -                return ret;
> +            bdrv_snapshot_goto(bs, name, &local_err);
> +            if (error_is_set(&local_err)) {
> +                qerror_report_err(local_err);

Shouldn't we add the device name on which the failure occurred?

> +                error_free(local_err);
> +                return -EIO;
>              }
>          }
>      }

Kevin