Re: [Qemu-devel] [PATCH] block/sheepdog: add error handling to sd_snapshot_delete()

[Jeff Cody <jcody@redhat.com>]

On Fri, Mar 18, 2016 at 05:54:38PM +0900, Takashi Menjo wrote:
> Errors have been ignored in some code paths in sd_snapshot_delete().
> This patch adds error handling.
> 
> Signed-off-by: Takashi Menjo <menjo.takashi@lab.ntt.co.jp>

Thank you for the patch!

> ---
>  block/sheepdog.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/block/sheepdog.c b/block/sheepdog.c
> index a3aeae4..6492405 100644
> --- a/block/sheepdog.c
> +++ b/block/sheepdog.c
> @@ -2565,6 +2565,7 @@ static int sd_snapshot_delete(BlockDriverState *bs,
>      SheepdogVdiRsp *rsp = (SheepdogVdiRsp *)&hdr;
>  
>      if (!remove_objects(s)) {
> +        error_report("failed to discard snapshot inode");

We want to set errp, so that the error is picked up correctly.  It is
assumed in QEMU that if there is an Error object passed, that it is
sufficient to check it for error (as opposed to checking the return
value).

You can use error_setg() here to do this, e.g.:

       error_setg(errp, "failed to discard snapshot inode");

>          return -1;
>      }
>  
> @@ -2588,6 +2589,7 @@ static int sd_snapshot_delete(BlockDriverState *bs,
>      ret = find_vdi_name(s, s->name, snap_id, snap_tag, &vid, true,
>                          &local_err);
>      if (ret) {
> +        error_report_err(local_err);

To propagate the local_err value to errp, use error_propagate:

    error_propagate(errp, local_err);

>          return ret;
>      }


There is another hunk that is missing an error_propagate in
sd_snapshot_delete:

2594     fd = connect_to_sdog(s, &local_err);
2595     if (fd < 0) {
2596         error_report_err(local_err);
2597         return -1;
2598     }
2599

>  
> @@ -2601,6 +2603,7 @@ static int sd_snapshot_delete(BlockDriverState *bs,
>                   buf, &wlen, &rlen);
>      closesocket(fd);
>      if (ret) {
> +        error_setg_errno(errp, -ret, "failed to delete %s", s->name);
>          return ret;
>      }

We also need to set errp in the switch statement on rsp->result:

2607     switch (rsp->result) {

[...]

2612     default:
2613         error_report("%s, %s", sd_strerror(rsp->result), s->name);
2614         return -1;
2615     }