Re: [Qemu-devel] [PATCH v3] avoid compilation warning/errors on up to date compilers/glibc

Re: [Qemu-devel] [PATCH v3] avoid compilation warning/errors on up to date compilers/glibc

[jcd]

----- "Anthony Liguori" <anthony@codemonkey.ws> a écrit :

> Jean-Christophe Dubois wrote:
> > Some system calls are now requiring to have their return value
> checked.
> >
> > Without this a warning is emitted and in the case a qemu an error
> is
> > triggered as qemu is considering warnings as errors.
> >
> > For example:
> >
> > block/cow.c: In function ‘cow_create’:
> > block/cow.c:251: error: ignoring return value of ‘write’, declared
> with
> > attribute warn_unused_result
> > block/cow.c:253: error: ignoring return value of ‘ftruncate’,
> declared
> > with attribute warn_unused_result
> >
> > This is an attempt at removing all these warnings to allow a clean
> > compilation with up to date compilers/distributions.
> >
> > The second version fixes an error detected by Stuart Brady as well
> > as some coding style issues. Note however that some of the
> > modified files don't follow the qemu coding style (using tabs
> > instead of spaces).
> >
> > The Third version add one ftruncate() system call error handling
> that
> > was missing from V2 (in block/vvfat.c).
> >
> > Signed-off-by: Jean-Christophe DUBOIS <jcd@tribudubois.net>
> >
> > ---
> >  block.c           |    3 ++-
> >  block/bochs.c     |    3 ++-
> >  block/cow.c       |   12 ++++++++++--
> >  block/qcow.c      |   22 ++++++++++++++++------
> >  block/qcow2.c     |   37 +++++++++++++++++++++++++++++--------
> >  block/raw-posix.c |    9 ++++++---
> >  block/vmdk.c      |   38 ++++++++++++++++++++++++++++----------
> >  block/vvfat.c     |   24 +++++++++++++++++-------
> >  linux-user/mmap.c |    7 +++++--
> >  linux-user/path.c |    6 +++++-
> >  osdep.c           |    5 ++++-
> >  slirp/misc.c      |    3 ++-
> >  usb-linux.c       |    3 +--
> >  vl.c              |   14 ++++++++++----
> >  14 files changed, 137 insertions(+), 49 deletions(-)
> >
> > diff --git a/block.c b/block.c
> > index aca5a6d..c78d66a 100644
> > --- a/block.c
> > +++ b/block.c
> > @@ -371,7 +371,8 @@ int bdrv_open2(BlockDriverState *bs, const char
> *filename, int flags,
> >              snprintf(backing_filename, sizeof(backing_filename),
> >                       "%s", filename);
> >          else
> > -            realpath(filename, backing_filename);
> > +            if (!realpath(filename, backing_filename))
> > +                return -1;
> >  
> >          bdrv_qcow2 = bdrv_find_format("qcow2");
> >          options = parse_option_parameters("",
> bdrv_qcow2->create_options, NULL);
> > diff --git a/block/bochs.c b/block/bochs.c
> > index bac81c4..0d614eb 100644
> > --- a/block/bochs.c
> > +++ b/block/bochs.c
> > @@ -199,7 +199,8 @@ static inline int
> seek_to_sector(BlockDriverState *bs, int64_t sector_num)
> >      // read in bitmap for current extent
> >      lseek(s->fd, bitmap_offset + (extent_offset / 8), SEEK_SET);
> >  
> > -    read(s->fd, &bitmap_entry, 1);
> > +    if (read(s->fd, &bitmap_entry, 1) != 1)
> > +        return -1; // not allocated
> >   
> 
> This is no more correct than before.  read() can return EINTR and that
> 
> should be handled appropriately.  Elsewhere, read() can return partial
> 
> results and we ought to handle that properly.

It handles all cases except EINTR. And in the actual code base EINTR and __all other__ error cases are not handled. So as of today EINTR is not handled propely (as well as all other error cases).
So this consider EINTR as an error when it could be handled in a nicer way. Previously no ERRORS were considered as an error. What is best?
 
> Using -D_FORTIFY_SOURCES=0 will eliminate these warnings.  If we're 
> going to fix these things, we should fix them properly.

It just hides the problem and fix nothing but the compilation. ERRORS will still be ignored/discarded without any warning. If the glibc designers are insisting on the fact that some system calls should have their return value checked there must be a reason. Is this really the best practice here (to just ignore errors)?
 
> Regards,
> 
> Anthony Liguori

Re: [Qemu-devel] [PATCH v3] avoid compilation warning/errors on up to date compilers/glibc

[Paul Brook]

> > This is no more correct than before.  read() can return EINTR and that
> > should be handled appropriately.  Elsewhere, read() can return partial
> > results and we ought to handle that properly.
>
> It handles all cases except EINTR. And in the actual code base EINTR and
> __all other__ error cases are not handled. So as of today EINTR is not
> handled propely (as well as all other error cases). So this consider EINTR
> as an error when it could be handled in a nicer way. Previously no ERRORS
> were considered as an error. What is best?

The original is the lesser wevil because it is obviously wrong. Your patch 
looks like it does error handling and avoids generates warnings, but still 
does not work correctty.

> > Using -D_FORTIFY_SOURCES=0 will eliminate these warnings.  If we're
> > going to fix these things, we should fix them properly.
>
> It just hides the problem and fix nothing but the compilation.

Your patch just makes the bug more subtle and harder to spot.

Paul

Re: [Qemu-devel] [PATCH v3] avoid compilation warning/errors on up to date compilers/glibc

[Jean-Christophe Dubois]

Le mercredi 17 juin 2009 16:03:51, vous avez écrit :
> > > This is no more correct than before.  read() can return EINTR and that
> > > should be handled appropriately.  Elsewhere, read() can return partial
> > > results and we ought to handle that properly.
> >
> > It handles all cases except EINTR. And in the actual code base EINTR and
> > __all other__ error cases are not handled. So as of today EINTR is not
> > handled propely (as well as all other error cases). So this consider
> > EINTR as an error when it could be handled in a nicer way. Previously no
> > ERRORS were considered as an error. What is best?
>
> The original is the lesser wevil because it is obviously wrong. Your patch
> looks like it does error handling and avoids generates warnings, but still
> does not work correctty.
>
> > > Using -D_FORTIFY_SOURCES=0 will eliminate these warnings.  If we're
> > > going to fix these things, we should fix them properly.
> >
> > It just hides the problem and fix nothing but the compilation.
>
> Your patch just makes the bug more subtle and harder to spot.

I guess this is a matter of point of view. 

Is it harder to find a bug when:
1) you are silencing compilation warning/errors AND ignoring all runtime 
errors (also silencing future warnings/errors).
or 2) You are considering recoverable errors as non recoverable errors.

You make your call.

Anyway fixing all those "bad" calls the "proper" way will be quite involved.

BTW are you sure that EINTR and EAGAIN are correctly handled all over the 
source code? What about partial read/write?

JC


>
> Paul

Re: [Qemu-devel] [PATCH v3] avoid compilation warning/errors on up to date compilers/glibc

[Luiz Capitulino]

On Wed, 17 Jun 2009 20:41:14 +0200
"Jean-Christophe Dubois" <jcd@tribudubois.net> wrote:

> Le mercredi 17 juin 2009 16:03:51, vous avez écrit :
> > > > This is no more correct than before.  read() can return EINTR
> > > > and that should be handled appropriately.  Elsewhere, read()
> > > > can return partial results and we ought to handle that properly.
> > >
> > > It handles all cases except EINTR. And in the actual code base
> > > EINTR and __all other__ error cases are not handled. So as of
> > > today EINTR is not handled propely (as well as all other error
> > > cases). So this consider EINTR as an error when it could be
> > > handled in a nicer way. Previously no ERRORS were considered as
> > > an error. What is best?
> >
> > The original is the lesser wevil because it is obviously wrong.
> > Your patch looks like it does error handling and avoids generates
> > warnings, but still does not work correctty.
> >
> > > > Using -D_FORTIFY_SOURCES=0 will eliminate these warnings.  If
> > > > we're going to fix these things, we should fix them properly.
> > >
> > > It just hides the problem and fix nothing but the compilation.
> >
> > Your patch just makes the bug more subtle and harder to spot.
> 
> I guess this is a matter of point of view. 

 I don't think it's a point of view, today we have a warning and we
know something is not ok. With this patch we won't have the warning
anymore but the solution is not what it should be.

 Can't we have wrappers to read() and write() that does the right
thing and use them everywhere?

> Is it harder to find a bug when:
> 1) you are silencing compilation warning/errors AND ignoring all
> runtime errors (also silencing future warnings/errors).

 My impression is that Anthony's tip is for you get it built, not
a final solution to the problem.

> or 2) You are considering recoverable errors as non recoverable
> errors.

 qemu_read() and qemu_write() could handle both.

> BTW are you sure that EINTR and EAGAIN are correctly handled all over
> the source code? What about partial read/write?

 I have no idea, but if they are not they should be fixed then.

Re: [Qemu-devel] [PATCH v3] avoid compilation warning/errors on up to date compilers/glibc

[jcd]

----- "Luiz Capitulino" <lcapitulino@redhat.com> a écrit :

>  I don't think it's a point of view, today we have a warning and we
> know something is not ok. With this patch we won't have the warning
> anymore but the solution is not what it should be.

If you silence GCC there will not be any warning anymore either.
 
>  Can't we have wrappers to read() and write() that does the right
> thing and use them everywhere?

It is already proposed. See version 4 of the patch.

Re: [Qemu-devel] [PATCH v3] avoid compilation warning/errors on up to date compilers/glibc

[Anthony Liguori]

Jean-Christophe Dubois wrote:
> Some system calls are now requiring to have their return value checked.
>
> Without this a warning is emitted and in the case a qemu an error is
> triggered as qemu is considering warnings as errors.
>
> For example:
>
> block/cow.c: In function ‘cow_create’:
> block/cow.c:251: error: ignoring return value of ‘write’, declared with
> attribute warn_unused_result
> block/cow.c:253: error: ignoring return value of ‘ftruncate’, declared
> with attribute warn_unused_result
>
> This is an attempt at removing all these warnings to allow a clean
> compilation with up to date compilers/distributions.
>
> The second version fixes an error detected by Stuart Brady as well
> as some coding style issues. Note however that some of the
> modified files don't follow the qemu coding style (using tabs
> instead of spaces).
>
> The Third version add one ftruncate() system call error handling that
> was missing from V2 (in block/vvfat.c).
>
> Signed-off-by: Jean-Christophe DUBOIS <jcd@tribudubois.net>
>
> ---
>  block.c           |    3 ++-
>  block/bochs.c     |    3 ++-
>  block/cow.c       |   12 ++++++++++--
>  block/qcow.c      |   22 ++++++++++++++++------
>  block/qcow2.c     |   37 +++++++++++++++++++++++++++++--------
>  block/raw-posix.c |    9 ++++++---
>  block/vmdk.c      |   38 ++++++++++++++++++++++++++++----------
>  block/vvfat.c     |   24 +++++++++++++++++-------
>  linux-user/mmap.c |    7 +++++--
>  linux-user/path.c |    6 +++++-
>  osdep.c           |    5 ++++-
>  slirp/misc.c      |    3 ++-
>  usb-linux.c       |    3 +--
>  vl.c              |   14 ++++++++++----
>  14 files changed, 137 insertions(+), 49 deletions(-)
>
> diff --git a/block.c b/block.c
> index aca5a6d..c78d66a 100644
> --- a/block.c
> +++ b/block.c
> @@ -371,7 +371,8 @@ int bdrv_open2(BlockDriverState *bs, const char *filename, int flags,
>              snprintf(backing_filename, sizeof(backing_filename),
>                       "%s", filename);
>          else
> -            realpath(filename, backing_filename);
> +            if (!realpath(filename, backing_filename))
> +                return -1;
>  
>          bdrv_qcow2 = bdrv_find_format("qcow2");
>          options = parse_option_parameters("", bdrv_qcow2->create_options, NULL);
> diff --git a/block/bochs.c b/block/bochs.c
> index bac81c4..0d614eb 100644
> --- a/block/bochs.c
> +++ b/block/bochs.c
> @@ -199,7 +199,8 @@ static inline int seek_to_sector(BlockDriverState *bs, int64_t sector_num)
>      // read in bitmap for current extent
>      lseek(s->fd, bitmap_offset + (extent_offset / 8), SEEK_SET);
>  
> -    read(s->fd, &bitmap_entry, 1);
> +    if (read(s->fd, &bitmap_entry, 1) != 1)
> +        return -1; // not allocated
>   

This is no more correct than before.  read() can return EINTR and that 
should be handled appropriately.  Elsewhere, read() can return partial 
results and we ought to handle that properly.

Using -D_FORTIFY_SOURCES=0 will eliminate these warnings.  If we're 
going to fix these things, we should fix them properly.

Regards,

Anthony Liguori

[Qemu-devel] [PATCH v3] avoid compilation warning/errors on up to date compilers/glibc

[Jean-Christophe Dubois]

Some system calls are now requiring to have their return value checked.

Without this a warning is emitted and in the case a qemu an error is
triggered as qemu is considering warnings as errors.

For example:

block/cow.c: In function ‘cow_create’:
block/cow.c:251: error: ignoring return value of ‘write’, declared with
attribute warn_unused_result
block/cow.c:253: error: ignoring return value of ‘ftruncate’, declared
with attribute warn_unused_result

This is an attempt at removing all these warnings to allow a clean
compilation with up to date compilers/distributions.

The second version fixes an error detected by Stuart Brady as well
as some coding style issues. Note however that some of the
modified files don't follow the qemu coding style (using tabs
instead of spaces).

The Third version add one ftruncate() system call error handling that
was missing from V2 (in block/vvfat.c).

Signed-off-by: Jean-Christophe DUBOIS <jcd@tribudubois.net>

---
 block.c           |    3 ++-
 block/bochs.c     |    3 ++-
 block/cow.c       |   12 ++++++++++--
 block/qcow.c      |   22 ++++++++++++++++------
 block/qcow2.c     |   37 +++++++++++++++++++++++++++++--------
 block/raw-posix.c |    9 ++++++---
 block/vmdk.c      |   38 ++++++++++++++++++++++++++++----------
 block/vvfat.c     |   24 +++++++++++++++++-------
 linux-user/mmap.c |    7 +++++--
 linux-user/path.c |    6 +++++-
 osdep.c           |    5 ++++-
 slirp/misc.c      |    3 ++-
 usb-linux.c       |    3 +--
 vl.c              |   14 ++++++++++----
 14 files changed, 137 insertions(+), 49 deletions(-)

diff --git a/block.c b/block.c
index aca5a6d..c78d66a 100644
--- a/block.c
+++ b/block.c
@@ -371,7 +371,8 @@ int bdrv_open2(BlockDriverState *bs, const char *filename, int flags,
             snprintf(backing_filename, sizeof(backing_filename),
                      "%s", filename);
         else
-            realpath(filename, backing_filename);
+            if (!realpath(filename, backing_filename))
+                return -1;
 
         bdrv_qcow2 = bdrv_find_format("qcow2");
         options = parse_option_parameters("", bdrv_qcow2->create_options, NULL);
diff --git a/block/bochs.c b/block/bochs.c
index bac81c4..0d614eb 100644
--- a/block/bochs.c
+++ b/block/bochs.c
@@ -199,7 +199,8 @@ static inline int seek_to_sector(BlockDriverState *bs, int64_t sector_num)
     // read in bitmap for current extent
     lseek(s->fd, bitmap_offset + (extent_offset / 8), SEEK_SET);
 
-    read(s->fd, &bitmap_entry, 1);
+    if (read(s->fd, &bitmap_entry, 1) != 1)
+        return -1; // not allocated
 
     if (!((bitmap_entry >> (extent_offset % 8)) & 1))
     {
diff --git a/block/cow.c b/block/cow.c
index 84818f1..c63920a 100644
--- a/block/cow.c
+++ b/block/cow.c
@@ -248,11 +248,19 @@ static int cow_create(const char *filename, QEMUOptionParameter *options)
     }
     cow_header.sectorsize = cpu_to_be32(512);
     cow_header.size = cpu_to_be64(image_sectors * 512);
-    write(cow_fd, &cow_header, sizeof(cow_header));
+    if (write(cow_fd, &cow_header, sizeof(cow_header)) != sizeof(cow_header))
+        goto fail;
+
     /* resize to include at least all the bitmap */
-    ftruncate(cow_fd, sizeof(cow_header) + ((image_sectors + 7) >> 3));
+    if (ftruncate(cow_fd, sizeof(cow_header) + ((image_sectors + 7) >> 3)))
+        goto fail;
+
     close(cow_fd);
     return 0;
+
+fail:
+    close(cow_fd);
+    return -1;
 }
 
 static void cow_flush(BlockDriverState *bs)
diff --git a/block/qcow.c b/block/qcow.c
index 55a68a6..fc581ec 100644
--- a/block/qcow.c
+++ b/block/qcow.c
@@ -801,17 +801,27 @@ static int qcow_create(const char *filename, QEMUOptionParameter *options)
     }
 
     /* write all the data */
-    write(fd, &header, sizeof(header));
-    if (backing_file) {
-        write(fd, backing_file, backing_filename_len);
-    }
-    lseek(fd, header_size, SEEK_SET);
+    if (write(fd, &header, sizeof(header)) != sizeof(header))
+        goto fail;
+
+    if (backing_file)
+        if (write(fd, backing_file, backing_filename_len) != backing_filename_len)
+            goto fail;
+
+    if (lseek(fd, header_size, SEEK_SET) == -1)
+        goto fail;
     tmp = 0;
     for(i = 0;i < l1_size; i++) {
-        write(fd, &tmp, sizeof(tmp));
+        if (write(fd, &tmp, sizeof(tmp)) != sizeof(tmp))
+            goto fail;
     }
+
     close(fd);
     return 0;
+
+fail:
+    close(fd);
+    return -1;
 }
 
 static int qcow_make_empty(BlockDriverState *bs)
diff --git a/block/qcow2.c b/block/qcow2.c
index 9acbddf..36383d2 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -742,7 +742,9 @@ static int qcow_create2(const char *filename, int64_t total_size,
         ref_clusters * s->cluster_size);
 
     /* write all the data */
-    write(fd, &header, sizeof(header));
+    if (write(fd, &header, sizeof(header)) != sizeof(header))
+        goto fail;
+
     if (backing_file) {
         if (backing_format_len) {
             char zero[16];
@@ -751,29 +753,48 @@ static int qcow_create2(const char *filename, int64_t total_size,
             memset(zero, 0, sizeof(zero));
             cpu_to_be32s(&ext_bf.magic);
             cpu_to_be32s(&ext_bf.len);
-            write(fd, &ext_bf, sizeof(ext_bf));
-            write(fd, backing_format, backing_format_len);
+            if (write(fd, &ext_bf, sizeof(ext_bf)) != sizeof(ext_bf))
+                goto fail;
+
+            if (write(fd, backing_format, backing_format_len) != backing_format_len)
+                goto fail;
+
             if (d>0) {
-                write(fd, zero, d);
+                if (write(fd, zero, d) != d)
+                    goto fail;
             }
         }
-        write(fd, backing_file, backing_filename_len);
+        if (write(fd, backing_file, backing_filename_len) != backing_filename_len)
+            goto fail;
     }
     lseek(fd, s->l1_table_offset, SEEK_SET);
     tmp = 0;
     for(i = 0;i < l1_size; i++) {
-        write(fd, &tmp, sizeof(tmp));
+        if (write(fd, &tmp, sizeof(tmp)) != sizeof(tmp))
+            goto fail;
     }
     lseek(fd, s->refcount_table_offset, SEEK_SET);
-    write(fd, s->refcount_table, s->cluster_size);
+    if (write(fd, s->refcount_table, s->cluster_size) != s->cluster_size)
+        goto fail;
 
     lseek(fd, s->refcount_block_offset, SEEK_SET);
-    write(fd, s->refcount_block, ref_clusters * s->cluster_size);
+    if (write(fd, s->refcount_block, ref_clusters * s->cluster_size) != ref_clusters * s->cluster_size)
+        goto fail;
 
     qemu_free(s->refcount_table);
+    s->refcount_table = NULL;
     qemu_free(s->refcount_block);
+    s->refcount_block = NULL;
     close(fd);
     return 0;
+
+fail:
+    qemu_free(s->refcount_table);
+    s->refcount_table = NULL;
+    qemu_free(s->refcount_block);
+    s->refcount_block = NULL;
+    close(fd);
+    return -1;
 }
 
 static int qcow_create(const char *filename, QEMUOptionParameter *options)
diff --git a/block/raw-posix.c b/block/raw-posix.c
index ccb014a..396120b 100644
--- a/block/raw-posix.c
+++ b/block/raw-posix.c
@@ -557,7 +557,8 @@ static void aio_signal_handler(int signum)
     if (posix_aio_state) {
         char byte = 0;
 
-        write(posix_aio_state->wfd, &byte, sizeof(byte));
+        if (write(posix_aio_state->wfd, &byte, sizeof(byte)) != sizeof(byte))
+            fprintf(stderr, "failed to write to posix_aio_state\n");
     }
 
     qemu_service_io();
@@ -837,6 +838,7 @@ static int raw_create(const char *filename, QEMUOptionParameter *options)
 {
     int fd;
     int64_t total_size = 0;
+    int ret = 0;
 
     /* Read out options */
     while (options && options->name) {
@@ -850,9 +852,10 @@ static int raw_create(const char *filename, QEMUOptionParameter *options)
               0644);
     if (fd < 0)
         return -EIO;
-    ftruncate(fd, total_size * 512);
+    if (ftruncate(fd, total_size * 512))
+        ret = -1;
     close(fd);
-    return 0;
+    return ret;
 }
 
 static void raw_flush(BlockDriverState *bs)
diff --git a/block/vmdk.c b/block/vmdk.c
index f21f02b..136d11b 100644
--- a/block/vmdk.c
+++ b/block/vmdk.c
@@ -233,7 +233,8 @@ static int vmdk_snapshot_create(const char *filename, const char *backing_file)
     memset(&header, 0, sizeof(header));
     memcpy(&header,&hdr[4], sizeof(header)); // skip the VMDK4_MAGIC
 
-    ftruncate(snp_fd, header.grain_offset << 9);
+    if (ftruncate(snp_fd, header.grain_offset << 9))
+        goto fail;
     /* the descriptor offset = 0x200 */
     if (lseek(p_fd, 0x200, SEEK_SET) == -1)
         goto fail;
@@ -771,22 +772,32 @@ static int vmdk_create(const char *filename, QEMUOptionParameter *options)
     header.check_bytes[3] = 0xa;
 
     /* write all the data */
-    write(fd, &magic, sizeof(magic));
-    write(fd, &header, sizeof(header));
+    if (write(fd, &magic, sizeof(magic)) != sizeof(magic))
+        goto fail;
 
-    ftruncate(fd, header.grain_offset << 9);
+    if (write(fd, &header, sizeof(header)) != sizeof(header))
+        goto fail;
+
+    if (ftruncate(fd, header.grain_offset << 9))
+        goto fail;
 
     /* write grain directory */
-    lseek(fd, le64_to_cpu(header.rgd_offset) << 9, SEEK_SET);
+    if (lseek(fd, le64_to_cpu(header.rgd_offset) << 9, SEEK_SET) == -1)
+        goto fail;
+
     for (i = 0, tmp = header.rgd_offset + gd_size;
          i < gt_count; i++, tmp += gt_size)
-        write(fd, &tmp, sizeof(tmp));
+        if (write(fd, &tmp, sizeof(tmp)) != sizeof(tmp))
+            goto fail;
 
     /* write backup grain directory */
-    lseek(fd, le64_to_cpu(header.gd_offset) << 9, SEEK_SET);
+    if (lseek(fd, le64_to_cpu(header.gd_offset) << 9, SEEK_SET) == -1)
+        goto fail;
+
     for (i = 0, tmp = header.gd_offset + gd_size;
          i < gt_count; i++, tmp += gt_size)
-        write(fd, &tmp, sizeof(tmp));
+        if (write(fd, &tmp, sizeof(tmp)) != sizeof(tmp))
+            goto fail;
 
     /* compose the descriptor */
     real_filename = filename;
@@ -802,11 +813,18 @@ static int vmdk_create(const char *filename, QEMUOptionParameter *options)
              total_size / (int64_t)(63 * 16));
 
     /* write the descriptor */
-    lseek(fd, le64_to_cpu(header.desc_offset) << 9, SEEK_SET);
-    write(fd, desc, strlen(desc));
+    if (lseek(fd, le64_to_cpu(header.desc_offset) << 9, SEEK_SET) == -1)
+        goto fail;
+
+    if (write(fd, desc, strlen(desc)) != strlen(desc))
+        goto fail;
 
     close(fd);
     return 0;
+
+fail:
+    close(fd);
+    return -1;
 }
 
 static void vmdk_close(BlockDriverState *bs)
diff --git a/block/vvfat.c b/block/vvfat.c
index 1e37b9f..3fe3a66 100644
--- a/block/vvfat.c
+++ b/block/vvfat.c
@@ -2215,6 +2215,7 @@ static int commit_one_file(BDRVVVFATState* s,
     char* cluster = qemu_malloc(s->cluster_size);
     uint32_t i;
     int fd = 0;
+    int ret = 0;
 
     assert(offset < size);
     assert((offset % s->cluster_size) == 0);
@@ -2229,14 +2230,15 @@ static int commit_one_file(BDRVVVFATState* s,
 	return fd;
     }
     if (offset > 0)
-	if (lseek(fd, offset, SEEK_SET) != offset)
-	    return -3;
+        if (lseek(fd, offset, SEEK_SET) != offset) {
+            ret = -3;
+            goto fail;
+        }
 
     while (offset < size) {
 	uint32_t c1;
 	int rest_size = (size - offset > s->cluster_size ?
 		s->cluster_size : size - offset);
-	int ret;
 
 	c1 = modified_fat_get(s, c);
 
@@ -2247,19 +2249,27 @@ static int commit_one_file(BDRVVVFATState* s,
 	    (uint8_t*)cluster, (rest_size + 0x1ff) / 0x200);
 
 	if (ret < 0)
-	    return ret;
+	    goto fail;
 
-	if (write(fd, cluster, rest_size) < 0)
-	    return -2;
+        if (write(fd, cluster, rest_size) < 0) {
+            ret = -2;
+            goto fail;
+        }
 
 	offset += rest_size;
 	c = c1;
     }
 
-    ftruncate(fd, size);
+    if (ftruncate(fd, size))
+        goto fail;
+
     close(fd);
 
     return commit_mappings(s, first_cluster, dir_index);
+
+fail:
+    close(fd);
+    return ret;
 }
 
 #ifdef DEBUG
diff --git a/linux-user/mmap.c b/linux-user/mmap.c
index aa22006..5a1b525 100644
--- a/linux-user/mmap.c
+++ b/linux-user/mmap.c
@@ -252,7 +252,8 @@ static int mmap_frag(abi_ulong real_start,
             mprotect(host_start, qemu_host_page_size, prot1 | PROT_WRITE);
 
         /* read the corresponding file data */
-        pread(fd, g2h(start), end - start, offset);
+        if (pread(fd, g2h(start), end - start, offset) == -1)
+            return -1;
 
         /* put final protection */
         if (prot_new != (prot1 | PROT_WRITE))
@@ -469,7 +470,9 @@ abi_long target_mmap(abi_ulong start, abi_ulong len, int prot,
                                   -1, 0);
             if (retaddr == -1)
                 goto fail;
-            pread(fd, g2h(start), len, offset);
+            if (pread(fd, g2h(start), len, offset) == -1)
+                goto fail;
+                
             if (!(prot & PROT_WRITE)) {
                 ret = target_mprotect(start, len, prot);
                 if (ret != 0) {
diff --git a/linux-user/path.c b/linux-user/path.c
index 06b1f5f..fc5cd6e 100644
--- a/linux-user/path.c
+++ b/linux-user/path.c
@@ -45,8 +45,12 @@ static struct pathelem *new_entry(const char *root,
 {
     struct pathelem *new = malloc(sizeof(*new));
     new->name = strdup(name);
-    asprintf(&new->pathname, "%s/%s", root, name);
     new->num_entries = 0;
+    if (asprintf(&new->pathname, "%s/%s", root, name) == -1) {
+        free(new->name);
+        free(new);
+        new = NULL;
+    }
     return new;
 }
 
diff --git a/osdep.c b/osdep.c
index b300ba1..de0124e 100644
--- a/osdep.c
+++ b/osdep.c
@@ -160,7 +160,10 @@ static void *kqemu_vmalloc(size_t size)
         unlink(phys_ram_file);
     }
     size = (size + 4095) & ~4095;
-    ftruncate(phys_ram_fd, phys_ram_size + size);
+    if (ftruncate(phys_ram_fd, phys_ram_size + size)) {
+        fprintf(stderr, "Could not truncate phys_ram_file\n");
+        exit(1);
+    }
 #endif /* !(__OpenBSD__ || __FreeBSD__ || __DragonFly__) */
     ptr = mmap(NULL,
                size,
diff --git a/slirp/misc.c b/slirp/misc.c
index 1391d49..e3f08e7 100644
--- a/slirp/misc.c
+++ b/slirp/misc.c
@@ -365,7 +365,8 @@ fork_exec(struct socket *so, const char *ex, int do_pty)
 			  snprintf(buff, sizeof(buff),
                                    "Error: execvp of %s failed: %s\n",
                                    argv[0], strerror(errno));
-			  write(2, buff, strlen(buff)+1);
+                          if (write(2, buff, strlen(buff)+1) != (strlen(buff)+1))
+                              lprint("Error: failed to write to stderr: %s\n", strerror(errno));
 		  }
 		close(0); close(1); close(2); /* XXX */
 		exit(1);
diff --git a/usb-linux.c b/usb-linux.c
index 67e4acd..d71c4b5 100644
--- a/usb-linux.c
+++ b/usb-linux.c
@@ -1159,9 +1159,8 @@ static int usb_host_read_file(char *line, size_t line_size, const char *device_f
              device_file);
     f = fopen(filename, "r");
     if (f) {
-        fgets(line, line_size, f);
+        if (fgets(line, line_size, f)) ret = 1;
         fclose(f);
-        ret = 1;
     } else {
         monitor_printf(mon, "husb: could not open %s\n", filename);
     }
diff --git a/vl.c b/vl.c
index 3242c23..68ecf00 100644
--- a/vl.c
+++ b/vl.c
@@ -3707,7 +3707,8 @@ static void qemu_event_increment(void)
     if (io_thread_fd == -1)
         return;
 
-    write(io_thread_fd, &byte, sizeof(byte));
+    if (write(io_thread_fd, &byte, sizeof(byte)) != sizeof(byte))
+        perror("Failed write");
 }
 
 static void qemu_event_read(void *opaque)
@@ -5785,7 +5786,8 @@ int main(int argc, char **argv, char **envp)
     if (pid_file && qemu_create_pidfile(pid_file) != 0) {
         if (daemonize) {
             uint8_t status = 1;
-            write(fds[1], &status, 1);
+            if (write(fds[1], &status, 1) != 1)
+                fprintf(stderr, "Could not write status to pid file \n");
         } else
             fprintf(stderr, "Could not acquire pid file\n");
         exit(1);
@@ -6216,7 +6218,9 @@ int main(int argc, char **argv, char **envp)
 	if (len != 1)
 	    exit(1);
 
-	chdir("/");
+        if (chdir("/"))
+            exit(1);
+
 	TFR(fd = open("/dev/null", O_RDWR));
 	if (fd == -1)
 	    exit(1);
@@ -6235,7 +6239,9 @@ int main(int argc, char **argv, char **envp)
             fprintf(stderr, "chroot failed\n");
             exit(1);
         }
-        chdir("/");
+
+        if (chdir("/"))
+            exit(1);
     }
 
     if (run_as) {