[Spice-devel] [RFC 2/3] server: introduce dispatcher

[Alon Levy]

On Wed, Nov 02, 2011 at 02:57:45PM +0100, Paolo Bonzini wrote:
> On 11/01/2011 09:10 AM, Alon Levy wrote:
> >+/*
> >+ * read_with_eintr
> >+ * helper. reads until size bytes accumulated in buf, if an error other then
> >+ * EINTR is encountered returns -1, otherwise returns 0.
> >+ */
> >+int read_with_eintr(int fd, void *buf, size_t size)
> >+{
> >+    int read_size = 0;
> >+    int ret;
> >+
> >+    while (read_size<  size) {
> >+        ret = read(fd, buf + read_size, size - read_size);
> >+        if (ret == -1) {
> >+            if (errno != EINTR) {
> >+                return -1;
> >+            }
> >+            continue;
> >+        }
> >+        read_size += size;
> >+    }
> >+    return 0;
> >+}
> 
> This fails if read returns zero.  It can do so for a broken pipe on
> the read side, even if fd is blocking.
> 

I mean to treat a broken pipe as a grave error. Not abort, since I think
spice shouldn't abort a vm if the display breaks, but otoh this should
be reported somehow at some point. Anyway, since this is an internal
pipe between two threads in the same process, I'm not sure how it can
possibly break, but if it does I don't know what else except shout
"evacuate ship" we can do?

> A more common name I've seen for this function is fullread or
> read_full (same for write).  For the version that just loops if
> EINTR, a more common name is saferead or read_safe.
> 

fair enough, thanks.

> >+    /* according to the man page EINTR will mean no data written, so
> >+     * no need to take account of short writes. */
> 
> write can certainly do short writes if fd's buffer is too small for
> "buf"!  It can also return zero if fd is in non-blocking mode, but
> I'm not sure that this is a problem for you.
> 

Per Yonit's comments I'm going to change the socketpair to non blocking,
so I'll update the function to handle short writes.

> >+    while (write(fd, buf, size) != size) {
> 
> If write does a short write, you'll write the beginning of buf
> multiple times, or possibly end up with an infinite loop even.
> 

Yes, I was aware of that, that's why I put that comment :) I'll be
fixing this.

> >+        if (errno != EINTR) {
> >+            return -1;
> >+        }
> >+    }
> 
> Paolo
> 

Thanks for the feedback.

> _______________________________________________
> Spice-devel mailing list
> Spice-devel at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/spice-devel