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:jc...@redhat.com]
> Sent: Saturday, March 19, 2016 1:17 AM
> To: Takashi Menjo <menjo.taka...@lab.ntt.co.jp>
> Cc: qemu-devel@nongnu.org; Hitoshi Mitake <mitake.hito...@lab.ntt.co.jp>;
> Vasiliy Tolstov <v.tols...@selfip.ru>; sheep...@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.taka...@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 }
>
>
>
