On Tue, Aug 07, 2012 at 03:43:09PM -0300, Erlon Cruz wrote:
> From: Erlon Cruz <erlon.cruz@xxxxxxxxxxxxxxxxxx>
>
> Signed-off-by: Erlon R. Cruz <erlon.cruz@xxxxxxxxxxxxxxxxxx>
> Signed-off-by: Fabiano Fidêncio <Fabiano.Fidêncio@xxxxxxxxxxxxxxxxxxxxx>
> Signed-off-by: Rafael F. Santos <Rafael.Santos@xxxxxxxxxxxxxxxxxxxxx>
>
> ---
> server/reds.c | 82 ++++++++++++++++++++++++++++++++++++++++++++++++---------
> 1 files changed, 69 insertions(+), 13 deletions(-)
>
> diff --git a/server/reds.c b/server/reds.c
> index e3ea154..9df6e2d 100644
> --- a/server/reds.c
> +++ b/server/reds.c
> @@ -36,6 +36,7 @@
> #include <errno.h>
> #include <ctype.h>
> #include <stdbool.h>
> +#include <endian.h>
>
> #include <openssl/bio.h>
> #include <openssl/pem.h>
> @@ -1216,6 +1217,23 @@ static void reds_channel_init_auth_caps(RedLinkInfo *link, RedChannel *channel)
> red_channel_set_common_cap(channel, SPICE_COMMON_CAP_PROTOCOL_AUTH_SELECTION);
> }
>
> +#define WORD32_MASK 0x03
Not sure about the name there
> +static inline void buf_swap_htole32(void *buf, size_t len){
This could be defined as a noop on LE machines
> +
> + uint32_t *pos = buf;
> +
> + if(WORD32_MASK&len){
> + printf("error swapping unaligned 32 bits word: len %d\n",(int)len);
Nor about the message wording here.
> + exit(0);
I don't think we should have the exit(0) always There. Maybe in the PPC
case, and maybe in debug, but otherwise a big error message should be
enough.
> + }
> +
> + while(len > 0){
> + *pos = htole32(*pos);
> + pos++;
> + len -= 4;
> + }
> +}
> +
> static int reds_send_link_ack(RedLinkInfo *link)
> {
> SpiceLinkHeader header;
> @@ -1224,12 +1242,12 @@ static int reds_send_link_ack(RedLinkInfo *link)
> RedChannelCapabilities *channel_caps;
> BUF_MEM *bmBuf;
> BIO *bio;
> - int ret = FALSE;
> + int ret = FALSE, *commom_caps = -1, *caps = -1;
commoN_caps
>
> header.magic = SPICE_MAGIC;
> header.size = sizeof(ack);
> - header.major_version = SPICE_VERSION_MAJOR;
> - header.minor_version = SPICE_VERSION_MINOR;
> + header.major_version = htole32(SPICE_VERSION_MAJOR);
> + header.minor_version = htole32(SPICE_VERSION_MINOR);
>
> ack.error = SPICE_LINK_ERR_OK;
>
> @@ -1243,10 +1261,12 @@ static int reds_send_link_ack(RedLinkInfo *link)
> reds_channel_init_auth_caps(link, channel); /* make sure common caps are set */
>
> channel_caps = &channel->local_caps;
> - ack.num_common_caps = channel_caps->num_common_caps;
> - ack.num_channel_caps = channel_caps->num_caps;
> - header.size += (ack.num_common_caps + ack.num_channel_caps) * sizeof(uint32_t);
> - ack.caps_offset = sizeof(SpiceLinkReply);
> + ack.num_common_caps = htole32(channel_caps->num_common_caps);
> + ack.num_channel_caps = htole32(channel_caps->num_caps);
> + header.size += (channel_caps->num_common_caps + channel_caps->num_caps) * sizeof(uint32_t);
> + ack.caps_offset = htole32(sizeof(SpiceLinkReply));
> +
> + header.size = htole32(header.size);
>
> if (!(link->tiTicketing.rsa = RSA_new())) {
> spice_warning("RSA nes failed");
> @@ -1264,8 +1284,20 @@ static int reds_send_link_ack(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));
>
> + commom_caps = spice_memdup(channel_caps->common_caps,
> + channel_caps->num_common_caps * sizeof(uint32_t));
> + buf_swap_htole32(commom_caps,
> + channel_caps->num_common_caps * sizeof(uint32_t));
> +
> + caps = spice_memdup(channel_caps->caps,
> + channel_caps->num_caps * sizeof(uint32_t));
> +
> + buf_swap_htole32(commom_caps,
> + channel_caps->num_caps * sizeof(uint32_t));
I think it would be better to write a sync_write equivalent which reads an
array 4 bytes at a time and swaps them before sending, this will avoid the
need for the memdup and the helper function.
> +
> if (!sync_write(link->stream, &header, sizeof(header)))
> goto end;
> if (!sync_write(link->stream, &ack, sizeof(ack)))
> @@ -1275,6 +1307,9 @@ static int reds_send_link_ack(RedLinkInfo *link)
> if (!sync_write(link->stream, channel_caps->caps, channel_caps->num_caps * sizeof(uint32_t)))
> goto end;
>
> + free(commom_caps);
> + free(caps);
> +
> ret = TRUE;
>
> end:
> @@ -1288,11 +1323,12 @@ static int reds_send_link_error(RedLinkInfo *link, uint32_t error)
> SpiceLinkReply reply;
>
> header.magic = SPICE_MAGIC;
> - header.size = sizeof(reply);
> - header.major_version = SPICE_VERSION_MAJOR;
> - header.minor_version = SPICE_VERSION_MINOR;
> + header.size = htole32(sizeof(reply));
> + header.major_version = htole32(SPICE_VERSION_MAJOR);
> + header.minor_version = htole32(SPICE_VERSION_MINOR);
> memset(&reply, 0, sizeof(reply));
> - reply.error = error;
> + reply.error = htole32(error);
> +
> return sync_write(link->stream, &header, sizeof(header)) && sync_write(link->stream, &reply,
> sizeof(reply));
> }
> @@ -1315,7 +1351,9 @@ 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));
> + uint32_t serror;
> + serror = htole32(error);
> + sync_write(link->stream, &serror, sizeof(serror));
> }
>
> int reds_expects_link_id(uint32_t connection_id)
> @@ -2337,6 +2375,8 @@ static void reds_handle_auth_mechanism(void *opaque)
> {
> RedLinkInfo *link = (RedLinkInfo *)opaque;
>
> + link->auth_mechanism.auth_mechanism = le32toh(link->auth_mechanism.auth_mechanism);
> +
> spice_info("Auth method: %d", link->auth_mechanism.auth_mechanism);
>
> if (link->auth_mechanism.auth_mechanism == SPICE_COMMON_CAP_AUTH_SPICE
> @@ -2371,9 +2411,17 @@ static void reds_handle_read_link_done(void *opaque)
> RedLinkInfo *link = (RedLinkInfo *)opaque;
> SpiceLinkMess *link_mess = link->link_mess;
> AsyncRead *obj = &link->asyc_read;
> +
> + link_mess->caps_offset = le32toh(link_mess->caps_offset);
> + link_mess->channel_id = link_mess->channel_id;
> + link_mess->channel_type = link_mess->channel_type;
These 2 assignments are not really useful ;)
> + link_mess->connection_id = le32toh(link_mess->connection_id);
> + link_mess->num_channel_caps = le32toh(link_mess->num_channel_caps);
> + link_mess->num_common_caps = le32toh(link_mess->num_common_caps);
> +
> uint32_t num_caps = link_mess->num_common_caps + link_mess->num_channel_caps;
> uint32_t *caps = (uint32_t *)((uint8_t *)link_mess + link_mess->caps_offset);
> - int auth_selection;
> + int auth_selection,i;
>
> if (num_caps && (num_caps * sizeof(uint32_t) + link_mess->caps_offset >
> link->link_header.size ||
> @@ -2383,6 +2431,9 @@ static void reds_handle_read_link_done(void *opaque)
> return;
> }
>
> + for(i = 0; i < num_caps;i++)
> + caps[i] = le32toh(caps[i]);
> +
> auth_selection = test_capabilty(caps, link_mess->num_common_caps,
> SPICE_COMMON_CAP_PROTOCOL_AUTH_SELECTION);
>
> @@ -2439,6 +2490,11 @@ static void reds_handle_read_header_done(void *opaque)
> SpiceLinkHeader *header = &link->link_header;
> AsyncRead *obj = &link->asyc_read;
>
> + header->magic = header->magic;
Not really useful either
> + header->major_version = le32toh(header->major_version);
> + header->minor_version = le32toh(header->minor_version);
> + header->size = le32toh(header->size);
> +
> if (header->magic != SPICE_MAGIC) {
> reds_send_link_error(link, SPICE_LINK_ERR_INVALID_MAGIC);
> reds_link_free(link);
The patch globally looks good, though it will probably need a few
iterations. I'm wondering if this stuff could be handled in spice1.proto
and the generic marshaller (but that's some kind of long term plan ;)
Christophe
Attachment:
pgpZVz2h4ox9W.pgp
Description: PGP signature
_______________________________________________ Spice-devel mailing list Spice-devel@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/spice-devel