| Message ID | 1407396520-2720-3-git-send-email-mitake.hitoshi@lab.ntt.co.jp |
|---|---|
| State | New |
| Headers | show |
On Thu, Aug 07, 2014 at 04:28:40PM +0900, Hitoshi Mitake wrote: > Recently, sheepdog revived its VDI locking functionality. This patch > updates sheepdog driver of QEMU for this feature: > > 1. Improve error message when QEMU fails to acquire lock of > VDI. Current sheepdog driver prints an error message "VDI isn't > locked" when it fails to acquire lock. It is a little bit confusing > because the mesage says VDI isn't locked but it is actually locked by > other VM. This patch modifies this confusing message. > > 2. Change error code for a case of failed locking. -EBUSY is a > suitable one. > > Reported-by: Valerio Pachera <sirio81@gmail.com> > Cc: Kevin Wolf <kwolf@redhat.com> > Cc: Stefan Hajnoczi <stefanha@redhat.com> > Cc: Liu Yuan <namei.unix@gmail.com> > Cc: MORITA Kazutaka <morita.kazutaka@lab.ntt.co.jp> > Signed-off-by: Hitoshi Mitake <mitake.hitoshi@lab.ntt.co.jp> > --- > block/sheepdog.c | 4 ++++ > 1 file changed, 4 insertions(+) > > diff --git a/block/sheepdog.c b/block/sheepdog.c > index 36f76f0..0b3f86d 100644 > --- a/block/sheepdog.c > +++ b/block/sheepdog.c > @@ -1112,9 +1112,13 @@ static int find_vdi_name(BDRVSheepdogState *s, const char *filename, > > if (rsp->result != SD_RES_SUCCESS) { > error_setg(errp, "cannot get vdi info, %s, %s %" PRIu32 " %s", > + rsp->result == SD_RES_VDI_NOT_LOCKED ? I'm puzzled by this check. we use SD_RES_VDI_LOCKED to indicate vid is already locked, no? > + "VDI is already locked by other VM" : > sd_strerror(rsp->result), filename, snapid, tag); > if (rsp->result == SD_RES_NO_VDI) { > ret = -ENOENT; > + } else if (rsp->result == SD_RES_VDI_NOT_LOCKED) { > + ret = -EBUSY; > } else { > ret = -EIO; > } It is better to use switch case to handle the result. Thanks Yuan
At Fri, 8 Aug 2014 13:31:39 +0800, Liu Yuan wrote: > > On Thu, Aug 07, 2014 at 04:28:40PM +0900, Hitoshi Mitake wrote: > > Recently, sheepdog revived its VDI locking functionality. This patch > > updates sheepdog driver of QEMU for this feature: > > > > 1. Improve error message when QEMU fails to acquire lock of > > VDI. Current sheepdog driver prints an error message "VDI isn't > > locked" when it fails to acquire lock. It is a little bit confusing > > because the mesage says VDI isn't locked but it is actually locked by > > other VM. This patch modifies this confusing message. > > > > 2. Change error code for a case of failed locking. -EBUSY is a > > suitable one. > > > > Reported-by: Valerio Pachera <sirio81@gmail.com> > > Cc: Kevin Wolf <kwolf@redhat.com> > > Cc: Stefan Hajnoczi <stefanha@redhat.com> > > Cc: Liu Yuan <namei.unix@gmail.com> > > Cc: MORITA Kazutaka <morita.kazutaka@lab.ntt.co.jp> > > Signed-off-by: Hitoshi Mitake <mitake.hitoshi@lab.ntt.co.jp> > > --- > > block/sheepdog.c | 4 ++++ > > 1 file changed, 4 insertions(+) > > > > diff --git a/block/sheepdog.c b/block/sheepdog.c > > index 36f76f0..0b3f86d 100644 > > --- a/block/sheepdog.c > > +++ b/block/sheepdog.c > > @@ -1112,9 +1112,13 @@ static int find_vdi_name(BDRVSheepdogState *s, const char *filename, > > > > if (rsp->result != SD_RES_SUCCESS) { > > error_setg(errp, "cannot get vdi info, %s, %s %" PRIu32 " %s", > > + rsp->result == SD_RES_VDI_NOT_LOCKED ? > > I'm puzzled by this check. > > we use SD_RES_VDI_LOCKED to indicate vid is already locked, no? We use SD_RES_VDI_NOT_LOCKED for indicating locking by this VM fails. > > > + "VDI is already locked by other VM" : > > sd_strerror(rsp->result), filename, snapid, tag); > > if (rsp->result == SD_RES_NO_VDI) { > > ret = -ENOENT; > > + } else if (rsp->result == SD_RES_VDI_NOT_LOCKED) { > > + ret = -EBUSY; > > } else { > > ret = -EIO; > > } > > It is better to use switch case to handle the result. using switch statement in this case only increases a number of lines of code: Current change: if (rsp->result == SD_RES_NO_VDI) { ret = -ENOENT; } else if (rsp->result == SD_RES_VDI_NOT_LOCKED) { ... Change with switch: switch (rsp->result) { case SD_RES_NO_VDI: ret = -ENOENT; break; case SD_RES_VDI_NOT_LOCKED: ... The change with switch statement requires one more line for break;. I think if statement is suitable for this case. Thanks, Hitoshi
On Fri, Aug 08, 2014 at 03:17:59PM +0900, Hitoshi Mitake wrote: > At Fri, 8 Aug 2014 13:31:39 +0800, > Liu Yuan wrote: > > > > On Thu, Aug 07, 2014 at 04:28:40PM +0900, Hitoshi Mitake wrote: > > > Recently, sheepdog revived its VDI locking functionality. This patch > > > updates sheepdog driver of QEMU for this feature: > > > > > > 1. Improve error message when QEMU fails to acquire lock of > > > VDI. Current sheepdog driver prints an error message "VDI isn't > > > locked" when it fails to acquire lock. It is a little bit confusing > > > because the mesage says VDI isn't locked but it is actually locked by > > > other VM. This patch modifies this confusing message. > > > > > > 2. Change error code for a case of failed locking. -EBUSY is a > > > suitable one. > > > > > > Reported-by: Valerio Pachera <sirio81@gmail.com> > > > Cc: Kevin Wolf <kwolf@redhat.com> > > > Cc: Stefan Hajnoczi <stefanha@redhat.com> > > > Cc: Liu Yuan <namei.unix@gmail.com> > > > Cc: MORITA Kazutaka <morita.kazutaka@lab.ntt.co.jp> > > > Signed-off-by: Hitoshi Mitake <mitake.hitoshi@lab.ntt.co.jp> > > > --- > > > block/sheepdog.c | 4 ++++ > > > 1 file changed, 4 insertions(+) > > > > > > diff --git a/block/sheepdog.c b/block/sheepdog.c > > > index 36f76f0..0b3f86d 100644 > > > --- a/block/sheepdog.c > > > +++ b/block/sheepdog.c > > > @@ -1112,9 +1112,13 @@ static int find_vdi_name(BDRVSheepdogState *s, const char *filename, > > > > > > if (rsp->result != SD_RES_SUCCESS) { > > > error_setg(errp, "cannot get vdi info, %s, %s %" PRIu32 " %s", > > > + rsp->result == SD_RES_VDI_NOT_LOCKED ? > > > > I'm puzzled by this check. > > > > we use SD_RES_VDI_LOCKED to indicate vid is already locked, no? > > We use SD_RES_VDI_NOT_LOCKED for indicating locking by this VM fails. > > > > > > + "VDI is already locked by other VM" : But this message said it was locked by others, and we have SD_RES_VDI_LOCKED for this case. We need fix sheep daemon for this case to return SD_RES_VDI_LOCKED for already locked case and NOT_LOCKED for other sheep internal errors. > > > sd_strerror(rsp->result), filename, snapid, tag); > > > if (rsp->result == SD_RES_NO_VDI) { > > > ret = -ENOENT; > > > + } else if (rsp->result == SD_RES_VDI_NOT_LOCKED) { > > > + ret = -EBUSY; > > > } else { > > > ret = -EIO; > > > } > > > > It is better to use switch case to handle the result. > > using switch statement in this case only increases a number of lines > of code: > > Current change: > if (rsp->result == SD_RES_NO_VDI) { > ret = -ENOENT; > } else if (rsp->result == SD_RES_VDI_NOT_LOCKED) { > ... > > Change with switch: > switch (rsp->result) { > case SD_RES_NO_VDI: > ret = -ENOENT; > break; > case SD_RES_VDI_NOT_LOCKED: > ... > > The change with switch statement requires one more line for break;. I > think if statement is suitable for this case. If you insist on 'if-else' over swtich case, it is fine with me. But I'd suggest switch-case because it looks cleaner and easier to understand if we have more than 2 branches. Thanks Yuan
At Fri, 8 Aug 2014 14:43:16 +0800, Liu Yuan wrote: > > On Fri, Aug 08, 2014 at 03:17:59PM +0900, Hitoshi Mitake wrote: > > At Fri, 8 Aug 2014 13:31:39 +0800, > > Liu Yuan wrote: > > > > > > On Thu, Aug 07, 2014 at 04:28:40PM +0900, Hitoshi Mitake wrote: > > > > Recently, sheepdog revived its VDI locking functionality. This patch > > > > updates sheepdog driver of QEMU for this feature: > > > > > > > > 1. Improve error message when QEMU fails to acquire lock of > > > > VDI. Current sheepdog driver prints an error message "VDI isn't > > > > locked" when it fails to acquire lock. It is a little bit confusing > > > > because the mesage says VDI isn't locked but it is actually locked by > > > > other VM. This patch modifies this confusing message. > > > > > > > > 2. Change error code for a case of failed locking. -EBUSY is a > > > > suitable one. > > > > > > > > Reported-by: Valerio Pachera <sirio81@gmail.com> > > > > Cc: Kevin Wolf <kwolf@redhat.com> > > > > Cc: Stefan Hajnoczi <stefanha@redhat.com> > > > > Cc: Liu Yuan <namei.unix@gmail.com> > > > > Cc: MORITA Kazutaka <morita.kazutaka@lab.ntt.co.jp> > > > > Signed-off-by: Hitoshi Mitake <mitake.hitoshi@lab.ntt.co.jp> > > > > --- > > > > block/sheepdog.c | 4 ++++ > > > > 1 file changed, 4 insertions(+) > > > > > > > > diff --git a/block/sheepdog.c b/block/sheepdog.c > > > > index 36f76f0..0b3f86d 100644 > > > > --- a/block/sheepdog.c > > > > +++ b/block/sheepdog.c > > > > @@ -1112,9 +1112,13 @@ static int find_vdi_name(BDRVSheepdogState *s, const char *filename, > > > > > > > > if (rsp->result != SD_RES_SUCCESS) { > > > > error_setg(errp, "cannot get vdi info, %s, %s %" PRIu32 " %s", > > > > + rsp->result == SD_RES_VDI_NOT_LOCKED ? > > > > > > I'm puzzled by this check. > > > > > > we use SD_RES_VDI_LOCKED to indicate vid is already locked, no? > > > > We use SD_RES_VDI_NOT_LOCKED for indicating locking by this VM fails. > > > > > > > > > + "VDI is already locked by other VM" : > > But this message said it was locked by others, and we have SD_RES_VDI_LOCKED for > this case. > > We need fix sheep daemon for this case to return SD_RES_VDI_LOCKED for already > locked case and NOT_LOCKED for other sheep internal errors. OK, I'll change it in v2. > > > > > sd_strerror(rsp->result), filename, snapid, tag); > > > > if (rsp->result == SD_RES_NO_VDI) { > > > > ret = -ENOENT; > > > > + } else if (rsp->result == SD_RES_VDI_NOT_LOCKED) { > > > > + ret = -EBUSY; > > > > } else { > > > > ret = -EIO; > > > > } > > > > > > It is better to use switch case to handle the result. > > > > using switch statement in this case only increases a number of lines > > of code: > > > > Current change: > > if (rsp->result == SD_RES_NO_VDI) { > > ret = -ENOENT; > > } else if (rsp->result == SD_RES_VDI_NOT_LOCKED) { > > ... > > > > Change with switch: > > switch (rsp->result) { > > case SD_RES_NO_VDI: > > ret = -ENOENT; > > break; > > case SD_RES_VDI_NOT_LOCKED: > > ... > > > > The change with switch statement requires one more line for break;. I > > think if statement is suitable for this case. > > If you insist on 'if-else' over swtich case, it is fine with me. But I'd suggest > switch-case because it looks cleaner and easier to understand if we have more > than 2 branches. Yes I think if-else is suitable for this case. It is easy for anybody to understand the above simple branch. Thanks, Hitoshi
diff --git a/block/sheepdog.c b/block/sheepdog.c index 36f76f0..0b3f86d 100644 --- a/block/sheepdog.c +++ b/block/sheepdog.c @@ -1112,9 +1112,13 @@ static int find_vdi_name(BDRVSheepdogState *s, const char *filename, if (rsp->result != SD_RES_SUCCESS) { error_setg(errp, "cannot get vdi info, %s, %s %" PRIu32 " %s", + rsp->result == SD_RES_VDI_NOT_LOCKED ? + "VDI is already locked by other VM" : sd_strerror(rsp->result), filename, snapid, tag); if (rsp->result == SD_RES_NO_VDI) { ret = -ENOENT; + } else if (rsp->result == SD_RES_VDI_NOT_LOCKED) { + ret = -EBUSY; } else { ret = -EIO; }
Recently, sheepdog revived its VDI locking functionality. This patch updates sheepdog driver of QEMU for this feature: 1. Improve error message when QEMU fails to acquire lock of VDI. Current sheepdog driver prints an error message "VDI isn't locked" when it fails to acquire lock. It is a little bit confusing because the mesage says VDI isn't locked but it is actually locked by other VM. This patch modifies this confusing message. 2. Change error code for a case of failed locking. -EBUSY is a suitable one. Reported-by: Valerio Pachera <sirio81@gmail.com> Cc: Kevin Wolf <kwolf@redhat.com> Cc: Stefan Hajnoczi <stefanha@redhat.com> Cc: Liu Yuan <namei.unix@gmail.com> Cc: MORITA Kazutaka <morita.kazutaka@lab.ntt.co.jp> Signed-off-by: Hitoshi Mitake <mitake.hitoshi@lab.ntt.co.jp> --- block/sheepdog.c | 4 ++++ 1 file changed, 4 insertions(+)