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

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

[Kevin Wolf]

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 <kwolf@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;
+        }
         bdrv_flush(bs);
     }
 
@@ -2082,9 +2085,11 @@ int bdrv_commit(BlockDriverState *bs)
      * Make sure all data we wrote to the backing device is actually
      * stable on disk.
      */
-    if (bs->backing_hd)
+    if (bs->backing_hd) {
         bdrv_flush(bs->backing_hd);
+    }
 
+    ret = 0;
 ro_cleanup:
     g_free(buf);
 
-- 
1.8.1.4

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

[Benoît Canet]

Le Friday 24 Jan 2014 à 14:08:21 (+0100), Kevin Wolf a écrit :
> 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 <kwolf@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;
> +        }
>          bdrv_flush(bs);
>      }
>  
> @@ -2082,9 +2085,11 @@ int bdrv_commit(BlockDriverState *bs)
>       * Make sure all data we wrote to the backing device is actually
>       * stable on disk.
>       */
> -    if (bs->backing_hd)
> +    if (bs->backing_hd) {
>          bdrv_flush(bs->backing_hd);
> +    }
>  
> +    ret = 0;
>  ro_cleanup:
>      g_free(buf);
>  
> -- 
> 1.8.1.4
> 
> 

the != 0 in "if (bdrv_read(bs, sector, buf, n) != 0) {" was really odd.

Reviewed-by: Benoit Canet <benoit@irqsave.net>

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

[Jeff Cody]

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 <kwolf@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

>          bdrv_flush(bs);
>      }
>  
> @@ -2082,9 +2085,11 @@ int bdrv_commit(BlockDriverState *bs)
>       * Make sure all data we wrote to the backing device is actually
>       * stable on disk.
>       */
> -    if (bs->backing_hd)
> +    if (bs->backing_hd) {
>          bdrv_flush(bs->backing_hd);
> +    }
>  
> +    ret = 0;
>  ro_cleanup:
>      g_free(buf);
>  
> -- 
> 1.8.1.4
> 

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 <kwolf@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.

Kevin

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 <kwolf@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