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

[Paolo Bonzini]

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 */
            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.

Paolo