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 <siri...@gmail.com>
> > > > Cc: Kevin Wolf <kw...@redhat.com>
> > > > Cc: Stefan Hajnoczi <stefa...@redhat.com>
> > > > Cc: Liu Yuan <namei.u...@gmail.com>
> > > > Cc: MORITA Kazutaka <morita.kazut...@lab.ntt.co.jp>
> > > > Signed-off-by: Hitoshi Mitake <mitake.hito...@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
