[Qemu-devel] [PATCH] suppress 'warn_unused_result' warning

[Qemu-devel] [PATCH] suppress 'warn_unused_result' warning

[Chih-Min Chao]

[-- Attachment #1.1: Type: text/plain, Size: 127 bytes --]

The patch add error handling to functions with 'warn_unused_result' return
value
such as write, read, ftruncate, and realpath.

[-- Attachment #1.2: Type: text/html, Size: 147 bytes --]

[-- Attachment #2: 0001-add-error-handling-to-suppress-warn_unused_result.patch --]
[-- Type: text/x-patch, Size: 11195 bytes --]

From 61851f65266eaafd4ff8775c097dafd7c2d3a2c3 Mon Sep 17 00:00:00 2001
From: Chih-Mih Chao <cmchao@gmail.com>
Date: Mon, 11 May 2009 02:57:05 +0800
Subject: [PATCH] add error handling to suppress 'warn_unused_result' warning

---
 block-bochs.c     |    3 ++-
 block-cow.c       |   11 +++++++++--
 block-qcow.c      |   17 ++++++++++++++---
 block-qcow2.c     |   38 ++++++++++++++++++++++++++++++--------
 block-raw-posix.c |    5 ++++-
 block-vmdk.c      |   26 +++++++++++++++++++-------
 block-vvfat.c     |    4 +++-
 block.c           |    3 ++-
 qemu-nbd.c        |    3 ++-
 usb-linux.c       |    4 +++-
 vl.c              |   18 ++++++++++++++----
 11 files changed, 102 insertions(+), 30 deletions(-)

diff --git a/block-bochs.c b/block-bochs.c
index 7a75412..97ec528 100644
--- a/block-bochs.c
+++ b/block-bochs.c
@@ -198,7 +198,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;
 
     if (!((bitmap_entry >> (extent_offset % 8)) & 1))
     {
diff --git a/block-cow.c b/block-cow.c
index 17e3292..c0e3911 100644
--- a/block-cow.c
+++ b/block-cow.c
@@ -239,11 +239,18 @@ static int cow_create(const char *filename, int64_t image_sectors,
     }
     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)) == -1)
+        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)) == -1)
+        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 fc6b809..16138f3 100644
--- a/block-qcow.c
+++ b/block-qcow.c
@@ -811,17 +811,28 @@ static int qcow_create(const char *filename, int64_t total_size,
     }
 
     /* write all the data */
-    write(fd, &header, sizeof(header));
+    if (write(fd, &header, sizeof(header)) == -1) {
+        goto fail;
+    }
+
     if (backing_file) {
-        write(fd, backing_file, backing_filename_len);
+        if (write(fd, backing_file, backing_filename_len) == -1) {
+            goto fail;
+        }
     }
     lseek(fd, header_size, SEEK_SET);
     tmp = 0;
     for(i = 0;i < l1_size; i++) {
-        write(fd, &tmp, sizeof(tmp));
+        if (write(fd, &tmp, sizeof(tmp)) == -1) {
+            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 9a49777..1306f42 100644
--- a/block-qcow2.c
+++ b/block-qcow2.c
@@ -1662,7 +1662,9 @@ static int qcow_create2(const char *filename, int64_t total_size,
     create_refcount_update(s, s->refcount_block_offset, ref_clusters * s->cluster_size);
 
     /* write all the data */
-    write(fd, &header, sizeof(header));
+    if (write(fd, &header, sizeof(header)) == -1) {
+        goto fail;
+    }
     if (backing_file) {
         if (backing_format_len) {
             char zero[16];
@@ -1671,29 +1673,49 @@ 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)) == -1) {
+                goto fail;
+            }
+            if (write(fd, backing_format, backing_format_len) == -1) {
+                goto fail;
+            }
             if (d>0) {
-                write(fd, zero, d);
+                if (write(fd, zero, d) == -1) {
+                    goto fail;
+                }
             }
         }
-        write(fd, backing_file, backing_filename_len);
+        if (write(fd, backing_file, backing_filename_len) == -1) {
+	    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)) == -1) {
+            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) == -1) {
+        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) == -1) {
+        goto fail;
+    }
 
     qemu_free(s->refcount_table);
     qemu_free(s->refcount_block);
     close(fd);
     return 0;
+
+fail:
+    qemu_free(s->refcount_table);
+    qemu_free(s->refcount_block);
+    close(fd);
+    return -1;
 }
 
 static int qcow_create(const char *filename, int64_t total_size,
diff --git a/block-raw-posix.c b/block-raw-posix.c
index 0663c06..3ace450 100644
--- a/block-raw-posix.c
+++ b/block-raw-posix.c
@@ -835,7 +835,10 @@ static int raw_create(const char *filename, int64_t total_size,
               0644);
     if (fd < 0)
         return -EIO;
-    ftruncate(fd, total_size * 512);
+    if (ftruncate(fd, total_size * 512) < 0) {
+        close(fd);
+        return -EIO;
+    }
     close(fd);
     return 0;
 }
diff --git a/block-vmdk.c b/block-vmdk.c
index d47d483..b880377 100644
--- a/block-vmdk.c
+++ b/block-vmdk.c
@@ -232,7 +232,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) == -1)
+        goto fail;
     /* the descriptor offset = 0x200 */
     if (lseek(p_fd, 0x200, SEEK_SET) == -1)
         goto fail;
@@ -756,22 +757,28 @@ static int vmdk_create(const char *filename, int64_t total_size,
     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)) == -1)
+        goto fail;
+    if (write(fd, &header, sizeof(header)) == -1)
+        goto fail;
 
-    ftruncate(fd, header.grain_offset << 9);
+    if (ftruncate(fd, header.grain_offset << 9) == -1)
+        goto fail;
 
     /* write grain directory */
     lseek(fd, le64_to_cpu(header.rgd_offset) << 9, SEEK_SET);
     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)) == -1)
+            goto fail;
 
     /* write backup grain directory */
     lseek(fd, le64_to_cpu(header.gd_offset) << 9, SEEK_SET);
     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)) == -1)
+            goto fail;
+
 
     /* compose the descriptor */
     real_filename = filename;
@@ -788,10 +795,15 @@ static int vmdk_create(const char *filename, int64_t total_size,
 
     /* write the descriptor */
     lseek(fd, le64_to_cpu(header.desc_offset) << 9, SEEK_SET);
-    write(fd, desc, strlen(desc));
+    if (write(fd, desc, strlen(desc)) == -1)
+        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 7905931..4de09ff 100644
--- a/block-vvfat.c
+++ b/block-vvfat.c
@@ -2256,7 +2256,9 @@ static int commit_one_file(BDRVVVFATState* s,
 	c = c1;
     }
 
-    ftruncate(fd, size);
+    if (ftruncate(fd, size) < 0)
+        return -4;
+
     close(fd);
 
     return commit_mappings(s, first_cluster, dir_index);
diff --git a/block.c b/block.c
index acb8976..55c64c6 100644
--- a/block.c
+++ b/block.c
@@ -394,7 +394,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) == NULL)
+                return -1;
 
         ret = bdrv_create2(&bdrv_qcow2, tmp_filename,
                            total_size, backing_filename, 
diff --git a/qemu-nbd.c b/qemu-nbd.c
index 0af97ca..b60602d 100644
--- a/qemu-nbd.c
+++ b/qemu-nbd.c
@@ -346,7 +346,8 @@ int main(int argc, char **argv)
         int sock;
 
         if (!verbose)
-            daemon(0, 0);	/* detach client and server */
+            if (daemon(0, 0) == -1)	/* detach client and server */
+                errx(errno, "Could not run in background");
 
         if (socket == NULL) {
             sprintf(sockpath, SOCKET_PATH, basename(device));
diff --git a/usb-linux.c b/usb-linux.c
index 70d7a1c..98e55b1 100644
--- a/usb-linux.c
+++ b/usb-linux.c
@@ -1160,7 +1160,9 @@ 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) == NULL)
+            monitor_printf(mon, "husb: could not read %s\n", filename);
+
         fclose(f);
         ret = 1;
     } else {
diff --git a/vl.c b/vl.c
index 8e4fba6..7e88ee1 100644
--- a/vl.c
+++ b/vl.c
@@ -3696,7 +3696,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)) == -1)
+        perror("Failed write io_thread");
 }
 
 static void qemu_event_read(void *opaque)
@@ -5629,7 +5630,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\n");
         } else
             fprintf(stderr, "Could not acquire pid file\n");
         exit(1);
@@ -6047,7 +6049,11 @@ int main(int argc, char **argv, char **envp)
 	if (len != 1)
 	    exit(1);
 
-	chdir("/");
+	if (chdir("/") == -1) {
+            fprintf(stderr, "change directory to '/' failed");
+            exit(1);
+        }
+
 	TFR(fd = open("/dev/null", O_RDWR));
 	if (fd == -1)
 	    exit(1);
@@ -6066,7 +6072,11 @@ int main(int argc, char **argv, char **envp)
             fprintf(stderr, "chroot failed\n");
             exit(1);
         }
-        chdir("/");
+
+	if (chdir("/") == -1) {
+            fprintf(stderr, "change directory to '/' failed");
+            exit(1);
+        }
     }
 
     if (run_as) {
-- 
1.6.0.4

Re: [Qemu-devel] [PATCH] suppress 'warn_unused_result' warning

[Paul Brook]

On Sunday 10 May 2009, Chih-Min Chao wrote:
> The patch add error handling to functions with 'warn_unused_result' return
> value
> such as write, read, ftruncate, and realpath.

I'm pretty sure this is wrong.  It feels a lot like blindly papering over 
compiler warnings without actually understanding what's going on.
In some cases errors are actually an expected part of normal operation. In 
many other cases you only handle complete failure, and not partial 
completion.

Paul

Re: [Qemu-devel] [PATCH] suppress 'warn_unused_result' warning

[Stuart Brady]

On Mon, May 11, 2009 at 03:15:11AM +0800, Chih-Min Chao wrote:
> The patch add error handling to functions with 'warn_unused_result' return
> value such as write, read, ftruncate, and realpath.

I'm slightly concerned -- read(), write() and ftruncate() can fail with
EINTR if a signal is received at an unfortunate time, can't they?

Do we mitigate this for the most part, somehow?

Do kernels try to avoid this behaviour?  If so, under what circumstances
might EINTR still be returned?

Is it acceptable to have a wrapper around these functions that retries
if the call fails with EINTR?

I would guess that for the most part, getting EINTR back is pretty rare,
as there's an awful lot of code (in QEMU and in other projects) that
doesn't check for it.

BTW, is it be possible for the write in qemu_event_increment() to
io_thread_fd to fail with EAGAIN?  If so, aborting with perror()
probably isn't right.
-- 
Stuart Brady

Re: [Qemu-devel] [PATCH] suppress 'warn_unused_result' warning

[Anthony Liguori]

Stuart Brady wrote:
> On Mon, May 11, 2009 at 03:15:11AM +0800, Chih-Min Chao wrote:
>   
>> The patch add error handling to functions with 'warn_unused_result' return
>> value such as write, read, ftruncate, and realpath.
>>     
>
> I'm slightly concerned -- read(), write() and ftruncate() can fail with
> EINTR if a signal is received at an unfortunate time, can't they?
>
> Do we mitigate this for the most part, somehow?
>
> Do kernels try to avoid this behaviour?  If so, under what circumstances
> might EINTR still be returned?
>   

EINTR is rather unfortunate.  A lot of libraries don't handle it well, 
for instance.

One the IO thread stabilizes and we can enable it by default, we'll be 
able to eliminate signaling in the IO thread and thereby eliminate all 
of the potentially buggy EINTR handling.

I think that's the best path forward.

NB we would still send the TCG thread signals to break it out of 
execution but the TCG thread should not generally be doing system calls.


Regards,

Anthony Liguori

Re: [Qemu-devel] [PATCH] suppress 'warn_unused_result' warning

[M. Warner Losh]

In message: <20090510221500.GA27879@miranda.arrow>
            Stuart Brady <sdbrady@ntlworld.com> writes:
: On Mon, May 11, 2009 at 03:15:11AM +0800, Chih-Min Chao wrote:
: > The patch add error handling to functions with 'warn_unused_result' return
: > value such as write, read, ftruncate, and realpath.
: 
: I'm slightly concerned -- read(), write() and ftruncate() can fail with
: EINTR if a signal is received at an unfortunate time, can't they?

It depends on how the signal mask for the system call is setup.

: Do we mitigate this for the most part, somehow?
: 
: Do kernels try to avoid this behaviour?  If so, under what circumstances
: might EINTR still be returned?

When a signal is received and you are waiting for data, you get
EINTR.  If there's data available, then I believe the behavior is to
return that data and not EINTR.  That's the way Unix works.

: Is it acceptable to have a wrapper around these functions that retries
: if the call fails with EINTR?
: 
: I would guess that for the most part, getting EINTR back is pretty rare,
: as there's an awful lot of code (in QEMU and in other projects) that
: doesn't check for it.

It is very rare...

: BTW, is it be possible for the write in qemu_event_increment() to
: io_thread_fd to fail with EAGAIN?  If so, aborting with perror()
: probably isn't right.

Warner

Re: [Qemu-devel] [PATCH] suppress 'warn_unused_result' warning

[Stuart Brady]

On Sun, May 10, 2009 at 07:53:35PM -0600, M. Warner Losh wrote:
> When a signal is received and you are waiting for data, you get
> EINTR.  If there's data available, then I believe the behavior is to
> return that data and not EINTR.  That's the way Unix works.

So if I do a read() from a file over NFS, and there's an awful lot of
latency (and perhaps even connection problems), and the process gets a
signal -- does that mean that the signal will only be delivered once
data is returned?

If not, then I would really start to wonder whether /all/ code dealing
with read(), write(), etc. should be written to cope with EINTR (and 
also partial reads/writes?) regardless of whatever is done with threads
and signal masks, as doing otherwise seems only to be asking for trouble
at some point.  (I'd be especially concerned about signals intended for
libraries that are not under the developer's control...)
-- 
Stuart Brady

Re: [Qemu-devel] [PATCH] suppress 'warn_unused_result' warning

[Paul Brook]

On Monday 11 May 2009, Stuart Brady wrote:
> On Sun, May 10, 2009 at 07:53:35PM -0600, M. Warner Losh wrote:
> > When a signal is received and you are waiting for data, you get
> > EINTR.  If there's data available, then I believe the behavior is to
> > return that data and not EINTR.  That's the way Unix works.
>
> So if I do a read() from a file over NFS, and there's an awful lot of
> latency (and perhaps even connection problems), and the process gets a
> signal -- does that mean that the signal will only be delivered once
> data is returned?

The signal should be delivered immediately. Depending on various factors the 
kernel will either restart the syscall, fail with EINTR or return a partial 
read.

> If not, then I would really start to wonder whether /all/ code dealing
> with read(), write(), etc. should be written to cope with EINTR (and
> also partial reads/writes?) regardless of whatever is done with threads
> and signal masks, as doing otherwise seems only to be asking for trouble
> at some point.  (I'd be especially concerned about signals intended for
> libraries that are not under the developer's control...)

If your application uses signals (directly or indirectly via libraries) then 
all uses of read/write need to be tolerant of EINTR and partial reads.

Paul

Re: [Qemu-devel] [PATCH] suppress 'warn_unused_result' warning

[Anthony Liguori]

Stuart Brady wrote:
> On Sun, May 10, 2009 at 07:53:35PM -0600, M. Warner Losh wrote:
>   
>> When a signal is received and you are waiting for data, you get
>> EINTR.  If there's data available, then I believe the behavior is to
>> return that data and not EINTR.  That's the way Unix works.
>>     
>
> So if I do a read() from a file over NFS, and there's an awful lot of
> latency (and perhaps even connection problems), and the process gets a
> signal -- does that mean that the signal will only be delivered once
> data is returned?
>
> If not, then I would really start to wonder whether /all/ code dealing
> with read(), write(), etc. should be written to cope with EINTR (and 
> also partial reads/writes?) regardless of whatever is done with threads
> and signal masks, as doing otherwise seems only to be asking for trouble
> at some point.  (I'd be especially concerned about signals intended for
> libraries that are not under the developer's control...)
>   

Any system call can return EINTR just about.  It's not just read/write.

In general, very few systems handle EINTR completely and QEMU is no 
exception here.

More should because SIGHUP is fairly common but I think that for the 
most part, it's sufficiently rare that it's not a problem for most projects.

Regards,

Anthony Liguori

Re: [Qemu-devel] [PATCH] suppress 'warn_unused_result' warning

[Daniel P. Berrange]

On Mon, May 11, 2009 at 11:16:24AM -0500, Anthony Liguori wrote:
> Stuart Brady wrote:
> >On Sun, May 10, 2009 at 07:53:35PM -0600, M. Warner Losh wrote:
> >  
> >>When a signal is received and you are waiting for data, you get
> >>EINTR.  If there's data available, then I believe the behavior is to
> >>return that data and not EINTR.  That's the way Unix works.
> >>    
> >
> >So if I do a read() from a file over NFS, and there's an awful lot of
> >latency (and perhaps even connection problems), and the process gets a
> >signal -- does that mean that the signal will only be delivered once
> >data is returned?
> >
> >If not, then I would really start to wonder whether /all/ code dealing
> >with read(), write(), etc. should be written to cope with EINTR (and 
> >also partial reads/writes?) regardless of whatever is done with threads
> >and signal masks, as doing otherwise seems only to be asking for trouble
> >at some point.  (I'd be especially concerned about signals intended for
> >libraries that are not under the developer's control...)
> >  
> 
> Any system call can return EINTR just about.  It's not just read/write.

For many system calls you can have them auto-restarted after EINTR
by using sigaction() with the SA_RESTART flag. IIRC read/write/poll 
won't support restarts in this way though, because of the problem of
partial data read/writes and partial timeouts for poll meaning you
can't auto-restart them without app developer help

Daniel
-- 
|: Red Hat, Engineering, London   -o-   http://people.redhat.com/berrange/ :|
|: http://libvirt.org  -o-  http://virt-manager.org  -o-  http://ovirt.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: GnuPG: 7D3B9505  -o-  F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|

Re: [Qemu-devel] [PATCH] suppress 'warn_unused_result' warning

[Anthony Liguori]

Daniel P. Berrange wrote:
> On Mon, May 11, 2009 at 11:16:24AM -0500, Anthony Liguori wrote:
>   
>> Any system call can return EINTR just about.  It's not just read/write.
>>     
>
> For many system calls you can have them auto-restarted after EINTR
> by using sigaction() with the SA_RESTART flag. IIRC read/write/poll 
> won't support restarts in this way though, because of the problem of
> partial data read/writes and partial timeouts for poll meaning you
> can't auto-restart them without app developer help
>   

SA_RESTART in theory is supposed to cover everything IIUC.  I don't know 
that that's true in Linux in practice though.  I definitely don't know 
about other Unices.

I don't think there's a standard way to know which syscalls would be 
restarted and which ones would.  If you expect EINTR, I think you pretty 
much have to handle it everywhere.

Regards,

Anthony Liguori

> Daniel
>   

Re: [Qemu-devel] [PATCH] suppress 'warn_unused_result' warning

[Jamie Lokier]

Anthony Liguori wrote:
> Daniel P. Berrange wrote:
> >On Mon, May 11, 2009 at 11:16:24AM -0500, Anthony Liguori wrote:
> >  
> >>Any system call can return EINTR just about.  It's not just read/write.
> >>    
> >
> >For many system calls you can have them auto-restarted after EINTR
> >by using sigaction() with the SA_RESTART flag. IIRC read/write/poll 
> >won't support restarts in this way though, because of the problem of
> >partial data read/writes and partial timeouts for poll meaning you
> >can't auto-restart them without app developer help
> >  
> 
> SA_RESTART in theory is supposed to cover everything IIUC.  I don't know 
> that that's true in Linux in practice though.  I definitely don't know 
> about other Unices.

It's not true on Linux - see "man 7 signal" which explains in some detail.
It's not true in general for particular system calls like select().

> I don't think there's a standard way to know which syscalls would be 
> restarted and which ones would.  If you expect EINTR, I think you pretty 
> much have to handle it everywhere.

I agree in practice.

-- Jamie

Re: [Qemu-devel] [PATCH] suppress 'warn_unused_result' warning

[Jamie Lokier]

Stuart Brady wrote:
> On Sun, May 10, 2009 at 07:53:35PM -0600, M. Warner Losh wrote:
> > When a signal is received and you are waiting for data, you get
> > EINTR.  If there's data available, then I believe the behavior is to
> > return that data and not EINTR.  That's the way Unix works.
> 
> So if I do a read() from a file over NFS, and there's an awful lot of
> latency (and perhaps even connection problems), and the process gets a
> signal -- does that mean that the signal will only be delivered once
> data is returned?

With NFS, if it's mounted "hard" it behaves like reading from a local
disk and returns exactly the number of bytes requested (unless it
reaches the end of file), and never returns EINTR.

If it's mounted "soft", it can return EINTR and does not have to wait
for data to be received.  But if it _already_ has some data when the
signal happens, it will return the amount of data it has already.

EINTR is not rare if you do a blocknig read from a pipe or socket, and
then receive a signal.  It returns EINTR every time :-)

> If not, then I would really start to wonder whether /all/ code dealing
> with read(), write(), etc. should be written to cope with EINTR (and 
> also partial reads/writes?) regardless of whatever is done with threads
> and signal masks, as doing otherwise seems only to be asking for trouble
> at some point.  (I'd be especially concerned about signals intended for
> libraries that are not under the developer's control...)

In some cases it's easier to set SA_RESTART when setting the signal
handler.  See "man 7 signal" and restarting signals - it has a good
explanation of when EINTR is received, when you can disable it, and
which systems calls you can't disable it for.

Of course signal handlers set by library code might not set that flag,
which is why it's usually not a good idea to rely on it.n

To be more thorough, do all I/O in thread swhich never receives
signals (except stop/kill).  This is achieved by pthread_sigmask() to
block all signals on those threads.  Make sure no libraries unmask them!

-- Jamie

Re: [Qemu-devel] [PATCH] suppress 'warn_unused_result' warning

[Daniel P. Berrange]

On Mon, May 11, 2009 at 03:15:11AM +0800, Chih-Min Chao wrote:
> The patch add error handling to functions with 'warn_unused_result' return
> value
> such as write, read, ftruncate, and realpath.

> @@ -239,11 +239,18 @@ static int cow_create(const char *filename, int64_t image_sectors,
>      }
>      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)) == -1)
> +        goto fail;

This isn't correct. You need to check that the write() actually 
wrote the number of bytes you asked it to., eg 

  if (write(cow_fd, &cow_header, sizeof(cow_header))  != sizeof(cow_header)
     goto fail;

would catch a short write, as well as other errors. Of course
you don't neccessarily want to fail on a short write, because
a reception of a signal can trigger a short write that can 
easily be recovered from by simply calling write() against for
the remainder of the data.

>      /* 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)) == -1)
> +        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 fc6b809..16138f3 100644
> --- a/block-qcow.c
> +++ b/block-qcow.c
> @@ -811,17 +811,28 @@ static int qcow_create(const char *filename, int64_t total_size,
>      }
>  
>      /* write all the data */
> -    write(fd, &header, sizeof(header));
> +    if (write(fd, &header, sizeof(header)) == -1) {
> +        goto fail;
> +    }
> +
>      if (backing_file) {
> -        write(fd, backing_file, backing_filename_len);
> +        if (write(fd, backing_file, backing_filename_len) == -1) {
> +            goto fail;
> +        }
>      }
>      lseek(fd, header_size, SEEK_SET);
>      tmp = 0;
>      for(i = 0;i < l1_size; i++) {
> -        write(fd, &tmp, sizeof(tmp));
> +        if (write(fd, &tmp, sizeof(tmp)) == -1) {
> +            goto fail;
> +        }
>      }

Likewise all these are failing to check for a complete write.

If we want to make this robust for EINTR too, then a small wrapper
around raw read/write calls would likely be wanted to deal with
fact an a signal can cause EINTR, *or* a short write

  ssize_t safewrite(int fd, const void *buf, size_t count)
  {
        size_t nwritten = 0;
        while (count > 0) {
                ssize_t r = write(fd, buf, count);

                if (r < 0 && errno == EINTR)
                        continue;
                if (r < 0)
                        return r;
                if (r == 0)
                        return nwritten;
                buf = (const char *)buf + r;
                count -= r;
                nwritten += r;
        }
        return nwritten;
  }


Regards,
Daniel
-- 
|: Red Hat, Engineering, London   -o-   http://people.redhat.com/berrange/ :|
|: http://libvirt.org  -o-  http://virt-manager.org  -o-  http://ovirt.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: GnuPG: 7D3B9505  -o-  F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|