On Tue, Mar 07, 2017 at 12:35:35PM +0000, Frediano Ziglio wrote:
> Send header and reply together.
> This potentially save up to 160 bytes on the network.
> This as sending multiple chunks you can have different framing,
> specifically:
> - if you use TLS every chunk get encrypted separately
> (reds-stream, currently usually 29 bytes for every chunks);
> - tcp settings and no delay on socket. More likely with fast
> connections or better network cards. The tcp framing is
> usually about 80 bytes;
> - additional tcp acknowledge (usually 64 bytes).
> So 80 + 29 + 64 = 173 bytes.
Thanks for the additional details! You made a comment on IRC about the
size of the 2 messages, which is about 50 bytes, I'd also mention it in
the commit log... (still to answer the same question, "Why are we doing
this, and why do we want to do it?")
(I could probably argue that this kind of buffering might be best done at a
lower level layer)
Acked-by: Christophe Fergeau <cfergeau@xxxxxxxxxx>
>
> Signed-off-by: Frediano Ziglio <fziglio@xxxxxxxxxx>
> ---
> server/reds.c | 58 +++++++++++++++++++++++++++++++---------------------------
> 1 file changed, 31 insertions(+), 27 deletions(-)
>
> Changes since v2:
> - explain the comment.
>
> Changes since v1:
> - added some comment;
> - check structure alignment.
>
>
> diff --git a/server/reds.c b/server/reds.c
> index fc720a3..41f24bb 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;
> RedChannel *channel;
> const RedChannelCapabilities *channel_caps;
> BUF_MEM *bmBuf;
> @@ -1468,12 +1470,14 @@ 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);
> + SPICE_VERIFY(sizeof(msg) == sizeof(SpiceLinkHeader) + sizeof(SpiceLinkReply));
>
> - ack.error = GUINT32_TO_LE(SPICE_LINK_ERR_OK);
> + 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);
> +
> + 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 +1493,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 +1524,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 +1536,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 +1562,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@xxxxxxxxxxxxxxxxxxxxx
> https://lists.freedesktop.org/mailman/listinfo/spice-devel
Attachment:
signature.asc
Description: PGP signature
_______________________________________________ Spice-devel mailing list Spice-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/spice-devel