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

[Kevin Wolf]

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.

Kevin