[Spice-devel] [PATCH spice-server] reds: Send link replies with less chunks

[Christophe Fergeau]

On Mon, Mar 06, 2017 at 04:12:33PM +0000, Frediano Ziglio wrote:
> Send header and reply together

The log is missing a "Why?"

> 
> Signed-off-by: Frediano Ziglio <fziglio at redhat.com>
> ---
>  server/reds.c | 56 +++++++++++++++++++++++++++++---------------------------
>  1 file changed, 29 insertions(+), 27 deletions(-)
> 
> diff --git a/server/reds.c b/server/reds.c
> index fc720a3..e94d0c4 100644
> --- a/server/reds.c
> +++ b/server/reds.c
> @@ -1459,8 +1459,10 @@ static bool red_link_info_test_capability(const RedLinkInfo *link, uint32_t cap)
>  
>  static int reds_send_link_ack(RedsState *reds, RedLinkInfo *link)
>  {
> -    SpiceLinkHeader header;
> -    SpiceLinkReply ack;
> +    struct {
> +        SpiceLinkHeader header;
> +        SpiceLinkReply ack;
> +    } msg;

I guess the compiler could decide to insert some padding between the 2
structs? If this happens, is this going to break the client/server
communication?

Christophe


>      RedChannel *channel;
>      const RedChannelCapabilities *channel_caps;
>      BUF_MEM *bmBuf;
> @@ -1468,12 +1470,12 @@ static int reds_send_link_ack(RedsState *reds, RedLinkInfo *link)
>      int ret = FALSE;
>      size_t hdr_size;
>  
> -    header.magic = SPICE_MAGIC;
> -    hdr_size = sizeof(ack);
> -    header.major_version = GUINT32_TO_LE(SPICE_VERSION_MAJOR);
> -    header.minor_version = GUINT32_TO_LE(SPICE_VERSION_MINOR);
> +    msg.header.magic = SPICE_MAGIC;
> +    hdr_size = sizeof(msg.ack);
> +    msg.header.major_version = GUINT32_TO_LE(SPICE_VERSION_MAJOR);
> +    msg.header.minor_version = GUINT32_TO_LE(SPICE_VERSION_MINOR);
>  
> -    ack.error = GUINT32_TO_LE(SPICE_LINK_ERR_OK);
> +    msg.ack.error = GUINT32_TO_LE(SPICE_LINK_ERR_OK);
>  
>      channel = reds_find_channel(reds, link->link_mess->channel_type,
>                                  link->link_mess->channel_id);
> @@ -1489,12 +1491,12 @@ static int reds_send_link_ack(RedsState *reds, RedLinkInfo *link)
>      reds_channel_init_auth_caps(link, channel); /* make sure common caps are set */
>  
>      channel_caps = red_channel_get_local_capabilities(channel);
> -    ack.num_common_caps = GUINT32_TO_LE(channel_caps->num_common_caps);
> -    ack.num_channel_caps = GUINT32_TO_LE(channel_caps->num_caps);
> +    msg.ack.num_common_caps = GUINT32_TO_LE(channel_caps->num_common_caps);
> +    msg.ack.num_channel_caps = GUINT32_TO_LE(channel_caps->num_caps);
>      hdr_size += channel_caps->num_common_caps * sizeof(uint32_t);
>      hdr_size += channel_caps->num_caps * sizeof(uint32_t);
> -    header.size = GUINT32_TO_LE(hdr_size);
> -    ack.caps_offset = GUINT32_TO_LE(sizeof(SpiceLinkReply));
> +    msg.header.size = GUINT32_TO_LE(hdr_size);
> +    msg.ack.caps_offset = GUINT32_TO_LE(sizeof(SpiceLinkReply));
>      if (!reds->config->sasl_enabled
>          || !red_link_info_test_capability(link, SPICE_COMMON_CAP_AUTH_SASL)) {
>          if (!(link->tiTicketing.rsa = RSA_new())) {
> @@ -1520,7 +1522,7 @@ static int reds_send_link_ack(RedsState *reds, RedLinkInfo *link)
>  
>          i2d_RSA_PUBKEY_bio(bio, link->tiTicketing.rsa);
>          BIO_get_mem_ptr(bio, &bmBuf);
> -        memcpy(ack.pub_key, bmBuf->data, sizeof(ack.pub_key));
> +        memcpy(msg.ack.pub_key, bmBuf->data, sizeof(msg.ack.pub_key));
>      } else {
>          /* if the client sets the AUTH_SASL cap, it indicates that it
>           * supports SASL, and will use it if the server supports SASL as
> @@ -1532,12 +1534,10 @@ static int reds_send_link_ack(RedsState *reds, RedLinkInfo *link)
>           * will fail.
>           */
>          spice_warning("not initialising RSA key");
> -        memset(ack.pub_key, '\0', sizeof(ack.pub_key));
> +        memset(msg.ack.pub_key, '\0', sizeof(msg.ack.pub_key));
>      }
>  
> -    if (!reds_stream_write_all(link->stream, &header, sizeof(header)))
> -        goto end;
> -    if (!reds_stream_write_all(link->stream, &ack, sizeof(ack)))
> +    if (!reds_stream_write_all(link->stream, &msg, sizeof(msg)))
>          goto end;
>      for (unsigned int i = 0; i < channel_caps->num_common_caps; i++) {
>          guint32 cap = GUINT32_TO_LE(channel_caps->common_caps[i]);
> @@ -1560,17 +1560,19 @@ end:
>  
>  static bool reds_send_link_error(RedLinkInfo *link, uint32_t error)
>  {
> -    SpiceLinkHeader header;
> -    SpiceLinkReply reply;
> -
> -    header.magic = SPICE_MAGIC;
> -    header.size = GUINT32_TO_LE(sizeof(reply));
> -    header.major_version = GUINT32_TO_LE(SPICE_VERSION_MAJOR);
> -    header.minor_version = GUINT32_TO_LE(SPICE_VERSION_MINOR);
> -    memset(&reply, 0, sizeof(reply));
> -    reply.error = GUINT32_TO_LE(error);
> -    return reds_stream_write_all(link->stream, &header, sizeof(header)) && reds_stream_write_all(link->stream, &reply,
> -                                                                         sizeof(reply));
> +    struct {
> +        SpiceLinkHeader header;
> +        SpiceLinkReply reply;
> +    } msg;
> +    SPICE_VERIFY(sizeof(msg) == sizeof(SpiceLinkHeader) + sizeof(SpiceLinkReply));
> +
> +    msg.header.magic = SPICE_MAGIC;
> +    msg.header.size = GUINT32_TO_LE(sizeof(msg.reply));
> +    msg.header.major_version = GUINT32_TO_LE(SPICE_VERSION_MAJOR);
> +    msg.header.minor_version = GUINT32_TO_LE(SPICE_VERSION_MINOR);
> +    memset(&msg.reply, 0, sizeof(msg.reply));
> +    msg.reply.error = GUINT32_TO_LE(error);
> +    return reds_stream_write_all(link->stream, &msg, sizeof(msg));
>  }
>  
>  static void reds_info_new_channel(RedLinkInfo *link, int connection_id)
> -- 
> 2.9.3
> 
> _______________________________________________
> Spice-devel mailing list
> Spice-devel at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/spice-devel
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 801 bytes
Desc: not available
URL: <https://lists.freedesktop.org/archives/spice-devel/attachments/20170306/3d158d4d/attachment-0001.sig>