[Spice-devel] [PATCH 05/13] server/reds: RFC: improve ssl_{read, write} return values

[Alon Levy]

On Tue, Feb 22, 2011 at 05:08:59PM +0100, Marc-André Lureau wrote:
> There is no known bug related to this patch, it might cause more harm
> than good.
> 
> The implementation of ssl_writev() looks incorrect: it never returned
> an error (return_code is always >= 0).

Actually it does - if the first call to write fails, then return_code==0,
so return_code<=0, and so it returns n.

> But, it should return the
> number of bytes written before it reached EAGAIN, otherwise, we risk
> of sending corrupted data.

It did that already.

If the first (or more then first) calls succeed, return_code would
accumulate the non zero n, so it will be positive, so return_code >0,
so ! return_code <= 0, and so when n <= 0 (the last failed call) it
will break, and then return return_code, the accumulated number of sent
bytes.

> 
> Finally, since the SSL usage is hidden from the reader/writter, and it
> assumes it is a fd-kind/libc of API (checking ERRNO), we should make
> sure errno is set to a correct value in that case (EAGAIN for instance).
> 
That (plus style changes) is what this patch adds. I have no idea if we
use errno somewhere, no idea if this change is good or bad. It looks good
looking at the callers (in red_channel.c red_peer_handle_outgoing for instance).

> Note: we should be careful with the polling of SSL fd, since
> SSL_read() or SSL_write() both can return WANT_READ or WANT_WRITE.  We
> should poll both IN and OUT, not sure how it's handled now.
me neither. Do you mean that after a WANT_READ we should only do a SSL_read and
not do a SSL_write?

> ---
>  server/reds.c |   31 +++++++++++++++++++------------
>  1 files changed, 19 insertions(+), 12 deletions(-)
> 
> diff --git a/server/reds.c b/server/reds.c
> index 45e4964..2aefe31 100644
> --- a/server/reds.c
> +++ b/server/reds.c
> @@ -319,6 +319,13 @@ static ssize_t stream_read_cb(RedsStream *s, void *buf, size_t size)
>      return read(s->socket, buf, size);
>  }
>  
> +static void ssl_error_eagain(int error)
> +{
> +    /* can this help? */
> +    if (error == SSL_ERROR_WANT_READ || error == SSL_ERROR_WANT_WRITE)
> +        errno = EAGAIN;
> +}
> +
>  static ssize_t stream_ssl_write_cb(RedsStream *s, const void *buf, size_t size)
>  {
>      int return_code;
> @@ -326,8 +333,10 @@ static ssize_t stream_ssl_write_cb(RedsStream *s, const void *buf, size_t size)
>  
>      return_code = SSL_write(s->ssl, buf, size);
>  
> -    if (return_code < 0)
> +    if (return_code < 0) {
>          ssl_error = SSL_get_error(s->ssl, return_code);
> +        ssl_error_eagain(ssl_error); /* just to be sure */
> +    }
>  
>      return return_code;
>  }
> @@ -339,8 +348,10 @@ static ssize_t stream_ssl_read_cb(RedsStream *s, void *buf, size_t size)
>  
>      return_code = SSL_read(s->ssl, buf, size);
>  
> -    if (return_code < 0)
> +    if (return_code < 0) {
>          ssl_error = SSL_get_error(s->ssl, return_code);
> +        ssl_error_eagain(ssl_error); /* just to be sure */
> +    }
>  
>      return return_code;
>  }
> @@ -349,24 +360,20 @@ static ssize_t stream_ssl_writev_cb(RedsStream *s, const struct iovec *vector, i
>  {
>      int i;
>      int n;
> -    ssize_t return_code = 0;
> +    ssize_t ret = 0;
>      int ssl_error;
>  
>      for (i = 0; i < count; ++i) {
>          n = SSL_write(s->ssl, vector[i].iov_base, vector[i].iov_len);
>          if (n <= 0) {
>              ssl_error = SSL_get_error(s->ssl, n);
> -            if (return_code <= 0) {
> -                return n;
> -            } else {
> -                break;
> -            }
> -        } else {
> -            return_code += n;
> -        }
> +            ssl_error_eagain(ssl_error); /* just to be sure we have EAGAIN */
> +            return ret == 0 ? n : ret;
> +        } else
> +            ret += n;
>      }
>  
> -    return return_code;
> +    return ret;
>  }
>  
>  static void reds_stream_remove_watch(RedsStream* s)
> -- 
> 1.7.4
> 
> _______________________________________________
> Spice-devel mailing list
> Spice-devel at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/spice-devel