[Spice-devel] [PATCH 4/5] server: introduce dispatcher

[Alon Levy]

On Mon, Nov 07, 2011 at 09:28:34AM +0100, Paolo Bonzini wrote:
> On 11/06/2011 05:49 PM, Alon Levy wrote:
> >+    while (read_size<  size) {
> >+        ret = read(fd, buf + read_size, size - read_size);
> >+        if (ret == -1) {
> >+            if (errno != EINTR) {
> >+                if (errno == EAGAIN&&  !block) {
> >+                    return 0;
> >+                }
> >+                return -1;
> >+            }
> >+            continue;
> >+        }
> >+        if (ret == 0) {
> >+            if (block) {
> >+                red_error("broken pipe on read");
> >+                return -1;
> 
> No, broken pipe on read will return 0 for non-blocking too.
> 
> The if should be removed, and the else branch below should be moved
> to errno == EAGAIN && block.
> 
> That is,
> 
>    if (fd_set_blocking(fd, block) == -1) {
>        return -1;
>    }
>    while (read_size < size) {
>        ret = read(fd, buf + read_size, size - read_size);
>        if (ret == -1) {
>            if (errno == EINTR) {
>                continue;
>            }
>            if (errno == EAGAIN) {
>                if (read_size == 0) {
>                    /* Nothing read so far, return 0.  */
>                    return 0;
>                }
>                if (fd_set_blocking(fd, 1) == -1) {
>                    return -1;
>                }
>                continue;
>            }
>            return -1;
>        }
>        if (ret == 0) {
>            red_error("broken pipe on read");
>            errno = ECONNRESET; /* example */

Why would I want to override errno?

>            return -1;
>        }
>        read_size += ret;
> 
> >+/*
> >+ * write_safe
> >+ * @return -1 for error, otherwise number of written bytes. may be zero.
> >+ */
> >+static int write_safe(int fd, void *buf, size_t size)
> >+{
> >+    int write_size = 0;
>            ^^^^^
> 
> Should be "written" (the "read" in read_size is pronounced "red",
> not "reed").
> 
> >+    int ret;
> >+
> >+    while (write_size<  size) {
> >+        ret = write(fd, buf + write_size, size - write_size);
> >+        if (ret == -1) {
> >+            if (errno != EINTR) {
> >+                return -1;
> >+            }
> >+            continue;
> >+        }
> >+        if (ret == 0) {
> >+            red_error("broken pipe on write");
> >+            return -1;
> 
> Broken pipe on write will return EPIPE or raise SIGPIPE (likely the
> former).  You can just remove the if altogether here.
> 
> However, you likely want to handle EAGAIN like in read_safe.
> Perhaps you need a block argument to write_safe as well.  If not,
> O_NONBLOCK applies to both read and write, so you should set it to a
> known invariant value---either at the end of read_safe, or at the
> beginning of write_safe.
> 
> Alternatively, instead of toggling the blocking flag, you can always
> leave it set and use a single poll system call in the !block case.
> If the descriptor is not readable (resp. writeable) you return 0
> immediately.

Taking this advice. Thanks for the multiple choice :)

> 
> Paolo