Re: [Qemu-devel] [PATCH] block: Fix bdrv_commit return value

[Jeff Cody]

On Fri, Jan 24, 2014 at 05:00:45PM +0100, Kevin Wolf wrote:
> Am 24.01.2014 um 16:56 hat Jeff Cody geschrieben:
> > On Fri, Jan 24, 2014 at 02:08:21PM +0100, Kevin Wolf wrote:
> > > bdrv_commit() could return 0 or 1 on success, depending on whether or
> > > now the last sector was allocated in the overlay and whether the overlay
> > > format had a .bdrv_make_empty callback.
> > > 
> > > Most callers ignored it, but qemu-img commit would print an error
> > > message while the operation actually succeeded.
> > > 
> > > Also clean up the handling of I/O errors to return the real error code
> > > instead of -EIO.
> > > 
> > > Signed-off-by: Kevin Wolf <address@hidden>
> > > ---
> > >  block.c | 15 ++++++++++-----
> > >  1 file changed, 10 insertions(+), 5 deletions(-)
> > > 
> > > diff --git a/block.c b/block.c
> > > index 3e0994b..7d22ca9 100644
> > > --- a/block.c
> > > +++ b/block.c
> > > @@ -2061,13 +2061,13 @@ int bdrv_commit(BlockDriverState *bs)
> > >              goto ro_cleanup;
> > >          }
> > >          if (ret) {
> > > -            if (bdrv_read(bs, sector, buf, n) != 0) {
> > > -                ret = -EIO;
> > > +            ret = bdrv_read(bs, sector, buf, n);
> > > +            if (ret < 0) {
> > >                  goto ro_cleanup;
> > >              }
> > >  
> > > -            if (bdrv_write(bs->backing_hd, sector, buf, n) != 0) {
> > > -                ret = -EIO;
> > > +            ret = bdrv_write(bs->backing_hd, sector, buf, n);
> > > +            if (ret < 0) {
> > >                  goto ro_cleanup;
> > >              }
> > >          }
> > > @@ -2075,6 +2075,9 @@ int bdrv_commit(BlockDriverState *bs)
> > >  
> > >      if (drv->bdrv_make_empty) {
> > >          ret = drv->bdrv_make_empty(bs);
> > > +        if (ret < 0) {
> > > +            goto ro_cleanup;
> > > +        }
> > 
> > QED has a .bdrv_make_empty implementation, but it is a stub that
> > always returns -ENOTSUP.
> > 
> > Prior to this patch, a commit of a QED snapshot would complete OK, but
> > return an error saying "Image is already committed" (Which almost
> > seems like a non-error error).
> > 
> > Now, we'll skip ahead to cleanup.
> > 
> > I think we should either: 1) filter out -ENOTSUP here, or 2) remove
> > the stub from QED
> 
> I vote for fixing QED, this stub doesn't make any sense and never
> matched the interface that was actually used. And I guess we can remove
> it in qcow2 as well, there it's a simple return 0 with the rest of the
> function #if'd out.
>

I agree, that would be cleanest approach, and would hopefully avoid
similar errors elsewhere later on.

Jeff