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 <kw...@redhat.com>
> > > ---
> > > 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
