Re: [PATCH 03/17] Move sync_write* to reds_stream.h

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On 01/07/2014 01:14 PM, Christophe Fergeau wrote:
> They are renamed to reds_stream_write*

You also changed the return type for reds_stream_write_all (was
sync_write) to bool, which is now being cast to int in some code paths.

Not an objection, but perhaps worth a later patch (unless it's already
addressed, I'm commenting while reading so haven't read the later
patches yet).

> ---
>  server/reds.c        | 78 ++++++++++++++++------------------------------------
>  server/reds_stream.c | 28 +++++++++++++++++++
>  server/reds_stream.h |  5 ++++
>  3 files changed, 57 insertions(+), 54 deletions(-)
> 
> diff --git a/server/reds.c b/server/reds.c
> index 81e90f3..ce8bb97 100644
> --- a/server/reds.c
> +++ b/server/reds.c
> @@ -1395,24 +1395,6 @@ int reds_handle_migrate_data(MainChannelClient *mcc, SpiceMigrateDataMain *mig_d
>      return TRUE;
>  }
>  
> -static int sync_write(RedsStream *stream, const void *in_buf, size_t n)
> -{
> -    const uint8_t *buf = (uint8_t *)in_buf;
> -
> -    while (n) {
> -        int now = reds_stream_write(stream, buf, n);
> -        if (now <= 0) {
> -            if (now == -1 && (errno == EINTR || errno == EAGAIN)) {
> -                continue;
> -            }
> -            return FALSE;
> -        }
> -        n -= now;
> -        buf += now;
> -    }
> -    return TRUE;
> -}
> -
>  static void reds_channel_init_auth_caps(RedLinkInfo *link, RedChannel *channel)
>  {
>      if (sasl_enabled && !link->skip_auth) {
> @@ -1473,13 +1455,13 @@ static int reds_send_link_ack(RedLinkInfo *link)
>      BIO_get_mem_ptr(bio, &bmBuf);
>      memcpy(ack.pub_key, bmBuf->data, sizeof(ack.pub_key));
>  
> -    if (!sync_write(link->stream, &header, sizeof(header)))
> +    if (!reds_stream_write_all(link->stream, &header, sizeof(header)))
>          goto end;
> -    if (!sync_write(link->stream, &ack, sizeof(ack)))
> +    if (!reds_stream_write_all(link->stream, &ack, sizeof(ack)))
>          goto end;
> -    if (!sync_write(link->stream, channel_caps->common_caps, channel_caps->num_common_caps * sizeof(uint32_t)))
> +    if (!reds_stream_write_all(link->stream, channel_caps->common_caps, channel_caps->num_common_caps * sizeof(uint32_t)))
>          goto end;
> -    if (!sync_write(link->stream, channel_caps->caps, channel_caps->num_caps * sizeof(uint32_t)))
> +    if (!reds_stream_write_all(link->stream, channel_caps->caps, channel_caps->num_caps * sizeof(uint32_t)))
>          goto end;
>  
>      ret = TRUE;
> @@ -1500,7 +1482,7 @@ static int reds_send_link_error(RedLinkInfo *link, uint32_t error)
>      header.minor_version = SPICE_VERSION_MINOR;
>      memset(&reply, 0, sizeof(reply));
>      reply.error = error;
> -    return sync_write(link->stream, &header, sizeof(header)) && sync_write(link->stream, &reply,
> +    return reds_stream_write_all(link->stream, &header, sizeof(header)) && reds_stream_write_all(link->stream, &reply,
>                                                                           sizeof(reply));
>  }
>  
> @@ -1522,7 +1504,7 @@ static void reds_info_new_channel(RedLinkInfo *link, int connection_id)
>  
>  static void reds_send_link_result(RedLinkInfo *link, uint32_t error)
>  {
> -    sync_write(link->stream, &error, sizeof(error));
> +    reds_stream_write_all(link->stream, &error, sizeof(error));
>  }
>  
>  int reds_expects_link_id(uint32_t connection_id)
> @@ -1977,18 +1959,6 @@ static inline void async_read_clear_handlers(AsyncRead *obj)
>      reds_stream_remove_watch(obj->stream);
>  }
>  
> -#if HAVE_SASL
> -static int sync_write_u8(RedsStream *s, uint8_t n)
> -{
> -    return sync_write(s, &n, sizeof(uint8_t));
> -}
> -
> -static int sync_write_u32(RedsStream *s, uint32_t n)
> -{
> -    return sync_write(s, &n, sizeof(uint32_t));
> -}
> -#endif
> -
>  static void async_read_handler(int fd, int event, void *data)
>  {
>      AsyncRead *obj = (AsyncRead *)data;
> @@ -2157,14 +2127,14 @@ static void reds_handle_auth_sasl_step(void *opaque)
>  
>      if (serveroutlen) {
>          serveroutlen += 1;
> -        sync_write(link->stream, &serveroutlen, sizeof(uint32_t));
> -        sync_write(link->stream, serverout, serveroutlen);
> +        reds_stream_write_all(link->stream, &serveroutlen, sizeof(uint32_t));
> +        reds_stream_write_all(link->stream, serverout, serveroutlen);
>      } else {
> -        sync_write(link->stream, &serveroutlen, sizeof(uint32_t));
> +        reds_stream_write_all(link->stream, &serveroutlen, sizeof(uint32_t));
>      }
>  
>      /* Whether auth is complete */
> -    sync_write_u8(link->stream, err == SASL_CONTINUE ? 0 : 1);
> +    reds_stream_write_u8(link->stream, err == SASL_CONTINUE ? 0 : 1);
>  
>      if (err == SASL_CONTINUE) {
>          spice_info("%s", "Authentication must continue (step)");
> @@ -2182,7 +2152,7 @@ static void reds_handle_auth_sasl_step(void *opaque)
>          }
>  
>          spice_info("Authentication successful");
> -        sync_write_u32(link->stream, SPICE_LINK_ERR_OK); /* Accept auth */
> +        reds_stream_write_u32(link->stream, SPICE_LINK_ERR_OK); /* Accept auth */
>  
>          /*
>           * Delay writing in SSF encoded until now
> @@ -2196,9 +2166,9 @@ static void reds_handle_auth_sasl_step(void *opaque)
>      return;
>  
>  authreject:
> -    sync_write_u32(link->stream, 1); /* Reject auth */
> -    sync_write_u32(link->stream, sizeof("Authentication failed"));
> -    sync_write(link->stream, "Authentication failed", sizeof("Authentication failed"));
> +    reds_stream_write_u32(link->stream, 1); /* Reject auth */
> +    reds_stream_write_u32(link->stream, sizeof("Authentication failed"));
> +    reds_stream_write_all(link->stream, "Authentication failed", sizeof("Authentication failed"));
>  
>  authabort:
>      reds_link_free(link);
> @@ -2288,14 +2258,14 @@ static void reds_handle_auth_sasl_start(void *opaque)
>  
>      if (serveroutlen) {
>          serveroutlen += 1;
> -        sync_write(link->stream, &serveroutlen, sizeof(uint32_t));
> -        sync_write(link->stream, serverout, serveroutlen);
> +        reds_stream_write_all(link->stream, &serveroutlen, sizeof(uint32_t));
> +        reds_stream_write_all(link->stream, serverout, serveroutlen);
>      } else {
> -        sync_write(link->stream, &serveroutlen, sizeof(uint32_t));
> +        reds_stream_write_all(link->stream, &serveroutlen, sizeof(uint32_t));
>      }
>  
>      /* Whether auth is complete */
> -    sync_write_u8(link->stream, err == SASL_CONTINUE ? 0 : 1);
> +    reds_stream_write_u8(link->stream, err == SASL_CONTINUE ? 0 : 1);
>  
>      if (err == SASL_CONTINUE) {
>          spice_info("%s", "Authentication must continue (start)");
> @@ -2313,7 +2283,7 @@ static void reds_handle_auth_sasl_start(void *opaque)
>          }
>  
>          spice_info("Authentication successful");
> -        sync_write_u32(link->stream, SPICE_LINK_ERR_OK); /* Accept auth */
> +        reds_stream_write_u32(link->stream, SPICE_LINK_ERR_OK); /* Accept auth */
>  
>          /*
>           * Delay writing in SSF encoded until now
> @@ -2327,9 +2297,9 @@ static void reds_handle_auth_sasl_start(void *opaque)
>      return;
>  
>  authreject:
> -    sync_write_u32(link->stream, 1); /* Reject auth */
> -    sync_write_u32(link->stream, sizeof("Authentication failed"));
> -    sync_write(link->stream, "Authentication failed", sizeof("Authentication failed"));
> +    reds_stream_write_u32(link->stream, 1); /* Reject auth */
> +    reds_stream_write_u32(link->stream, sizeof("Authentication failed"));
> +    reds_stream_write_all(link->stream, "Authentication failed", sizeof("Authentication failed"));
>  
>  authabort:
>      reds_link_free(link);
> @@ -2529,8 +2499,8 @@ static void reds_start_auth_sasl(RedLinkInfo *link)
>      sasl->mechlist = spice_strdup(mechlist);
>  
>      mechlistlen = strlen(mechlist);
> -    if (!sync_write(link->stream, &mechlistlen, sizeof(uint32_t))
> -        || !sync_write(link->stream, sasl->mechlist, mechlistlen)) {
> +    if (!reds_stream_write_all(link->stream, &mechlistlen, sizeof(uint32_t))
> +        || !reds_stream_write_all(link->stream, sasl->mechlist, mechlistlen)) {
>          spice_warning("SASL mechanisms write error");
>          goto error;
>      }
> diff --git a/server/reds_stream.c b/server/reds_stream.c
> index 7adc745..093621f 100644
> --- a/server/reds_stream.c
> +++ b/server/reds_stream.c
> @@ -57,6 +57,24 @@ ssize_t reds_stream_read(RedsStream *s, void *buf, size_t nbyte)
>      return ret;
>  }
>  
> +bool reds_stream_write_all(RedsStream *stream, const void *in_buf, size_t n)
> +{
> +    const uint8_t *buf = (uint8_t *)in_buf;
> +
> +    while (n) {
> +        int now = reds_stream_write(stream, buf, n);
> +        if (now <= 0) {
> +            if (now == -1 && (errno == EINTR || errno == EAGAIN)) {
> +                continue;
> +            }
> +            return FALSE;
> +        }
> +        n -= now;
> +        buf += now;
> +    }
> +    return TRUE;
> +}
> +
>  static ssize_t reds_stream_sasl_write(RedsStream *s, const void *buf, size_t nbyte);
>  
>  ssize_t reds_stream_write(RedsStream *s, const void *buf, size_t nbyte)
> @@ -132,6 +150,16 @@ void reds_stream_push_channel_event(RedsStream *s, int event)
>  }
>  
>  #if HAVE_SASL
> +bool reds_stream_write_u8(RedsStream *s, uint8_t n)
> +{
> +    return reds_stream_write_all(s, &n, sizeof(uint8_t));
> +}
> +
> +bool reds_stream_write_u32(RedsStream *s, uint32_t n)
> +{
> +    return reds_stream_write_all(s, &n, sizeof(uint32_t));
> +}
> +
>  static ssize_t reds_stream_sasl_write(RedsStream *s, const void *buf, size_t nbyte)
>  {
>      ssize_t ret;
> diff --git a/server/reds_stream.h b/server/reds_stream.h
> index ca18f75..c725414 100644
> --- a/server/reds_stream.h
> +++ b/server/reds_stream.h
> @@ -21,6 +21,8 @@
>  #include "spice.h"
>  #include "common/mem.h"
>  
> +#include <stdbool.h>
> +
>  #include <openssl/ssl.h>
>  
>  #if HAVE_SASL
> @@ -92,6 +94,9 @@ typedef enum {
>  ssize_t reds_stream_read(RedsStream *s, void *buf, size_t nbyte);
>  ssize_t reds_stream_write(RedsStream *s, const void *buf, size_t nbyte);
>  ssize_t reds_stream_writev(RedsStream *s, const struct iovec *iov, int iovcnt);
> +bool reds_stream_write_all(RedsStream *stream, const void *in_buf, size_t n);
> +bool reds_stream_write_u8(RedsStream *s, uint8_t n);
> +bool reds_stream_write_u32(RedsStream *s, uint32_t n);
>  void reds_stream_free(RedsStream *s);
>  
>  void reds_stream_push_channel_event(RedsStream *s, int event);
> 

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




[Index of Archives]     [Linux ARM Kernel]     [Linux ARM]     [Linux Omap]     [Fedora ARM]     [IETF Annouce]     [Security]     [Bugtraq]     [Linux]     [Linux OMAP]     [Linux MIPS]     [ECOS]     [Asterisk Internet PBX]     [Linux API]     [Monitors]