Patchwork [2/2] check for close() errors on qcow2_create()

login
register
mail settings
Submitter Eduardo Habkost
Date Oct. 7, 2010, 8:25 p.m.
Message ID <1286483105-9768-3-git-send-email-ehabkost@redhat.com>
Download mbox | patch
Permalink /patch/67104/
State New
Headers show

Comments

Eduardo Habkost - Oct. 7, 2010, 8:25 p.m.
Errors when closing the file we just created should not be ignored. I/O errors
may happen and "qemu-img create" should fail in those cases.

If we are already exiting due to an error, we will still return the original
error number, though.

Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
 block/qcow2.c |    6 ++++--
 1 files changed, 4 insertions(+), 2 deletions(-)
Stefan Hajnoczi - Oct. 8, 2010, 9:28 a.m.
On Thu, Oct 7, 2010 at 9:25 PM, Eduardo Habkost <ehabkost@redhat.com> wrote:
> Errors when closing the file we just created should not be ignored. I/O errors
> may happen and "qemu-img create" should fail in those cases.
>
> If we are already exiting due to an error, we will still return the original
> error number, though.
>
> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> ---
>  block/qcow2.c |    6 ++++--
>  1 files changed, 4 insertions(+), 2 deletions(-)

Sounds good.

> diff --git a/block/qcow2.c b/block/qcow2.c
> index c5fb28e..d3a056b 100644
> --- a/block/qcow2.c
> +++ b/block/qcow2.c
> @@ -882,7 +882,7 @@ static int qcow_create2(const char *filename, int64_t total_size,
>     uint64_t old_ref_clusters;
>     QCowCreateState s1, *s = &s1;
>     QCowExtension ext_bf = {0, 0};
> -    int ret;
> +    int ret, cret;
>
>     memset(s, 0, sizeof(*s));
>
> @@ -1055,7 +1055,9 @@ exit:
>     qemu_free(s->refcount_block);
>
>  exit_close:
> -    close(fd);
> +    cret = close(fd);
> +    if (ret == 0 && cret < 0)

if (close(fd) < 0 && ret == 0) {

Does the same without variable cret.

> +        ret = -errno;
>
>     /* Preallocate metadata */
>     if (ret == 0 && prealloc) {
> --
> 1.6.5.5
>
>
>
Kevin Wolf - Oct. 8, 2010, 10:14 a.m.
Am 07.10.2010 22:25, schrieb Eduardo Habkost:
> Errors when closing the file we just created should not be ignored. I/O errors
> may happen and "qemu-img create" should fail in those cases.
> 
> If we are already exiting due to an error, we will still return the original
> error number, though.
> 
> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> ---
>  block/qcow2.c |    6 ++++--
>  1 files changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/block/qcow2.c b/block/qcow2.c
> index c5fb28e..d3a056b 100644
> --- a/block/qcow2.c
> +++ b/block/qcow2.c
> @@ -882,7 +882,7 @@ static int qcow_create2(const char *filename, int64_t total_size,
>      uint64_t old_ref_clusters;
>      QCowCreateState s1, *s = &s1;
>      QCowExtension ext_bf = {0, 0};
> -    int ret;
> +    int ret, cret;
>  
>      memset(s, 0, sizeof(*s));
>  
> @@ -1055,7 +1055,9 @@ exit:
>      qemu_free(s->refcount_block);
>  
>  exit_close:
> -    close(fd);
> +    cret = close(fd);
> +    if (ret == 0 && cret < 0)
> +        ret = -errno;

Braces are missing here.

Kevin
Eduardo Habkost - Oct. 8, 2010, 3:29 p.m.
On Fri, Oct 08, 2010 at 10:28:37AM +0100, Stefan Hajnoczi wrote:
> >  exit_close:
> > -    close(fd);
> > +    cret = close(fd);
> > +    if (ret == 0 && cret < 0)
> 
> if (close(fd) < 0 && ret == 0) {
> 
> Does the same without variable cret.

Yes. I used the variable just for readability. I personally don't like
having side-effects inside a if condition.

Patch

diff --git a/block/qcow2.c b/block/qcow2.c
index c5fb28e..d3a056b 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -882,7 +882,7 @@  static int qcow_create2(const char *filename, int64_t total_size,
     uint64_t old_ref_clusters;
     QCowCreateState s1, *s = &s1;
     QCowExtension ext_bf = {0, 0};
-    int ret;
+    int ret, cret;
 
     memset(s, 0, sizeof(*s));
 
@@ -1055,7 +1055,9 @@  exit:
     qemu_free(s->refcount_block);
 
 exit_close:
-    close(fd);
+    cret = close(fd);
+    if (ret == 0 && cret < 0)
+        ret = -errno;
 
     /* Preallocate metadata */
     if (ret == 0 && prealloc) {