* [Qemu-devel] [PATCH] block/sheepdog: add error handling to sd_snapshot_delete()
@ 2016-03-18 8:54 Takashi Menjo
2016-03-18 16:17 ` Jeff Cody
0 siblings, 1 reply; 3+ messages in thread
From: Takashi Menjo @ 2016-03-18 8:54 UTC (permalink / raw)
To: qemu-devel
Cc: Hitoshi Mitake, Takashi Menjo, Jeff Cody, sheepdog, Vasiliy Tolstov
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>
---
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");
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);
return ret;
}
@@ -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;
}
--
2.7.4.windows.1
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [Qemu-devel] [PATCH] block/sheepdog: add error handling to sd_snapshot_delete()
2016-03-18 8:54 [Qemu-devel] [PATCH] block/sheepdog: add error handling to sd_snapshot_delete() Takashi Menjo
@ 2016-03-18 16:17 ` Jeff Cody
2016-03-22 3:59 ` MENJO, Takashi
0 siblings, 1 reply; 3+ messages in thread
From: Jeff Cody @ 2016-03-18 16:17 UTC (permalink / raw)
To: Takashi Menjo; +Cc: Hitoshi Mitake, sheepdog, qemu-devel, Vasiliy Tolstov
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 }
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [Qemu-devel] [PATCH] block/sheepdog: add error handling to sd_snapshot_delete()
2016-03-18 16:17 ` Jeff Cody
@ 2016-03-22 3:59 ` MENJO, Takashi
0 siblings, 0 replies; 3+ messages in thread
From: MENJO, Takashi @ 2016-03-22 3:59 UTC (permalink / raw)
To: 'Jeff Cody'
Cc: 'Hitoshi Mitake',
sheepdog, qemu-devel, 'Vasiliy Tolstov'
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 }
>
>
>
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2016-03-22 4:00 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-03-18 8:54 [Qemu-devel] [PATCH] block/sheepdog: add error handling to sd_snapshot_delete() Takashi Menjo
2016-03-18 16:17 ` Jeff Cody
2016-03-22 3:59 ` MENJO, Takashi
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.