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

[Frediano Ziglio]

> 
> On Mon, Mar 06, 2017 at 04:12:33PM +0000, Frediano Ziglio wrote:
> > Send header and reply together
> 
> The log is missing a "Why?"
> 

Potentially to safe up to 160 bytes!

> > 
> > 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
> 

I can copy the same paranoid check:

SPICE_VERIFY(sizeof(msg) == sizeof(SpiceLinkHeader) + sizeof(SpiceLinkReply));

Note that these structures are packed ones so compiler would
break its ABI.

> 
> >      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)

Frediano