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

["MENJO, Takashi" <menjo.takashi@lab.ntt.co.jp>]

Thank you for your review, Jeff!
I'll submit a patch v2 soon.

In addition, I found that we also need to set errp below.
This will be fixed in v2, too.
| 2607    switch (rsp->result) {
| 2608    case SD_RES_NO_VDI:
| 2609        error_report("%s was already deleted", s->name);


Takashi

> -----Original Message-----
> From: Jeff Cody [mailto:jcody@redhat.com]
> Sent: Saturday, March 19, 2016 1:17 AM
> To: Takashi Menjo <menjo.takashi@lab.ntt.co.jp>
> Cc: qemu-devel@nongnu.org; Hitoshi Mitake <mitake.hitoshi@lab.ntt.co.jp>;
> Vasiliy Tolstov <v.tolstov@selfip.ru>; sheepdog@lists.wpkg.org
> Subject: Re: [PATCH] block/sheepdog: add error handling to
> sd_snapshot_delete()
> 
> 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     }
> 
> 
>