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

[Liu Yuan <namei.unix@gmail.com>]

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