[sheepdog] [PATCH 2/2] sheepdog: improve error handling for a case of failed lock

[Hitoshi Mitake]

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 at gmail.com>
> > > > Cc: Kevin Wolf <kwolf at redhat.com>
> > > > Cc: Stefan Hajnoczi <stefanha at redhat.com>
> > > > Cc: Liu Yuan <namei.unix at gmail.com>
> > > > Cc: MORITA Kazutaka <morita.kazutaka at lab.ntt.co.jp>
> > > > Signed-off-by: Hitoshi Mitake <mitake.hitoshi at 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