Acked-by: Christophe Fergeau <cferg...@redhat.com>

On Wed, May 16, 2018 at 06:00:35PM +0100, Frediano Ziglio wrote:
> The Smartcard channel definition has been always broken.
> Multiple client messages with the same ID are defined in the channel.
> This cause on server demarshaller to only have last message defined,
> while on the client marshaller code all message marshallers are
> defined but client uses only header message.
> 
> Following the difference of the generated code.
> 
>   diff -rup old/generated_client_marshallers.c 
> common/generated_client_marshallers.c
>   --- old/generated_client_marshallers.c      2018-05-14 22:49:07.641778414 
> +0100
>   +++ common/generated_client_marshallers.c   2018-05-14 22:49:22.266329296 
> +0100
>   @@ -389,27 +389,6 @@ static void spice_marshall_msgc_tunnel_s
>    }
> 
>    #ifdef USE_SMARTCARD
>   -static void spice_marshall_msgc_smartcard_data(SPICE_GNUC_UNUSED 
> SpiceMarshaller *m, SPICE_GNUC_UNUSED SpiceMsgcSmartcard *msg, 
> SpiceMarshaller **reader_name_out)
>   -{
>   -    SPICE_GNUC_UNUSED SpiceMarshaller *m2;
>   -    SpiceMsgcSmartcard *src;
>   -    *reader_name_out = NULL;
>   -    src = (SpiceMsgcSmartcard *)msg;
>   -
>   -    /* header */ {
>   -        spice_marshaller_add_uint32(m, src->header.type);
>   -        spice_marshaller_add_uint32(m, src->header.reader_id);
>   -        spice_marshaller_add_uint32(m, src->header.length);
>   -    }
>   -    if (src->header.type == SPICE_VSC_MESSAGE_TYPE_ReaderAdd) {
>   -        /* Don't marshall @nomarshal reader_name */
>   -    } else if (src->header.type == SPICE_VSC_MESSAGE_TYPE_ATR || 
> src->header.type == SPICE_VSC_MESSAGE_TYPE_APDU) {
>   -        /* Remaining data must be appended manually */
>   -    } else if (src->header.type == SPICE_VSC_MESSAGE_TYPE_Error) {
>   -        spice_marshaller_add_uint32(m, src->error.code);
>   -    }
>   -}
>   -
>    static void spice_marshall_msgc_smartcard_header(SPICE_GNUC_UNUSED 
> SpiceMarshaller *m, SPICE_GNUC_UNUSED VSCMsgHeader *msg)
>    {
>        SPICE_GNUC_UNUSED SpiceMarshaller *m2;
>   @@ -421,25 +400,6 @@ static void spice_marshall_msgc_smartcar
>        spice_marshaller_add_uint32(m, src->length);
>    }
> 
>   -static void spice_marshall_msgc_smartcard_error(SPICE_GNUC_UNUSED 
> SpiceMarshaller *m, SPICE_GNUC_UNUSED VSCMsgError *msg)
>   -{
>   -    SPICE_GNUC_UNUSED SpiceMarshaller *m2;
>   -    VSCMsgError *src;
>   -    src = (VSCMsgError *)msg;
>   -
>   -    spice_marshaller_add_uint32(m, src->code);
>   -}
>   -
>   -static void spice_marshall_msgc_smartcard_atr(SPICE_GNUC_UNUSED 
> SpiceMarshaller *m, SPICE_GNUC_UNUSED VSCMsgATR *msg)
>   -{
>   -    SPICE_GNUC_UNUSED SpiceMarshaller *m2;
>   -}
>   -
>   -static void spice_marshall_msgc_smartcard_reader_add(SPICE_GNUC_UNUSED 
> SpiceMarshaller *m, SPICE_GNUC_UNUSED VSCMsgReaderAdd *msg)
>   -{
>   -    SPICE_GNUC_UNUSED SpiceMarshaller *m2;
>   -}
>   -
>    #endif /* USE_SMARTCARD */
>    static void spice_marshall_SpiceMsgCompressedData(SPICE_GNUC_UNUSED 
> SpiceMarshaller *m, SPICE_GNUC_UNUSED SpiceMsgCompressedData *msg)
>    {
>   @@ -496,20 +456,8 @@ SpiceMessageMarshallers * spice_message_
>        marshallers.msgc_record_mode = spice_marshall_msgc_record_mode;
>        marshallers.msgc_record_start_mark = 
> spice_marshall_msgc_record_start_mark;
>    #ifdef USE_SMARTCARD
>   -    marshallers.msgc_smartcard_atr = spice_marshall_msgc_smartcard_atr;
>   -#endif /* USE_SMARTCARD */
>   -#ifdef USE_SMARTCARD
>   -    marshallers.msgc_smartcard_data = spice_marshall_msgc_smartcard_data;
>   -#endif /* USE_SMARTCARD */
>   -#ifdef USE_SMARTCARD
>   -    marshallers.msgc_smartcard_error = spice_marshall_msgc_smartcard_error;
>   -#endif /* USE_SMARTCARD */
>   -#ifdef USE_SMARTCARD
>        marshallers.msgc_smartcard_header = 
> spice_marshall_msgc_smartcard_header;
>    #endif /* USE_SMARTCARD */
>   -#ifdef USE_SMARTCARD
>   -    marshallers.msgc_smartcard_reader_add = 
> spice_marshall_msgc_smartcard_reader_add;
>   -#endif /* USE_SMARTCARD */
>        marshallers.msgc_tunnel_service_add = 
> spice_marshall_msgc_tunnel_service_add;
>        marshallers.msgc_tunnel_service_remove = 
> spice_marshall_msgc_tunnel_service_remove;
>        marshallers.msgc_tunnel_socket_closed = 
> spice_marshall_msgc_tunnel_socket_closed;
>   diff -rup old/generated_client_marshallers.h 
> common/generated_client_marshallers.h
>   --- old/generated_client_marshallers.h      2018-05-14 22:49:07.641778414 
> +0100
>   +++ common/generated_client_marshallers.h   2018-05-14 22:49:22.739358627 
> +0100
>   @@ -61,11 +61,7 @@ typedef struct {
>        void (*msgc_tunnel_socket_data)(SpiceMarshaller *m, 
> SpiceMsgcTunnelSocketData *msg);
>        void (*msgc_tunnel_socket_token)(SpiceMarshaller *m, 
> SpiceMsgcTunnelSocketTokens *msg);
>    #ifdef USE_SMARTCARD
>   -    void (*msgc_smartcard_data)(SpiceMarshaller *m, SpiceMsgcSmartcard 
> *msg, SpiceMarshaller **reader_name_out);
>        void (*msgc_smartcard_header)(SpiceMarshaller *m, VSCMsgHeader *msg);
>   -    void (*msgc_smartcard_error)(SpiceMarshaller *m, VSCMsgError *msg);
>   -    void (*msgc_smartcard_atr)(SpiceMarshaller *m, VSCMsgATR *msg);
>   -    void (*msgc_smartcard_reader_add)(SpiceMarshaller *m, VSCMsgReaderAdd 
> *msg);
>    #endif /* USE_SMARTCARD */
>        void (*msg_SpiceMsgCompressedData)(SpiceMarshaller *m, 
> SpiceMsgCompressedData *msg);
>        void (*msgc_port_event)(SpiceMarshaller *m, SpiceMsgcPortEvent *msg);
>   Only in common/: generated_client_marshallers.lo
>   diff -rup old/generated_server_demarshallers.c 
> common/generated_server_demarshallers.c
>   --- old/generated_server_demarshallers.c    2018-05-14 22:49:07.641778414 
> +0100
>   +++ common/generated_server_demarshallers.c 2018-05-14 22:49:23.498405695 
> +0100
>   @@ -1957,24 +1957,18 @@ static uint8_t * parse_TunnelChannel_msg
> 
>    #ifdef USE_SMARTCARD
> 
>   -static uint8_t * parse_msgc_smartcard_reader_add(uint8_t *message_start, 
> uint8_t *message_end, SPICE_GNUC_UNUSED int minor, size_t *size, 
> message_destructor_t *free_message)
>   +static uint8_t * parse_msgc_smartcard_header(uint8_t *message_start, 
> uint8_t *message_end, SPICE_GNUC_UNUSED int minor, size_t *size, 
> message_destructor_t *free_message)
>    {
>        SPICE_GNUC_UNUSED uint8_t *pos;
>        uint8_t *start = message_start;
>        uint8_t *data = NULL;
>        uint64_t nw_size;
>   +    uint64_t mem_size;
>        uint8_t *in, *end;
>   -    uint64_t reader_name__nw_size;
>   -    uint64_t reader_name__nelements;
>   -    VSCMsgReaderAdd *out;
>   +    VSCMsgHeader *out;
> 
>   -    { /* reader_name */
>   -        reader_name__nelements = message_end - (start + 0);
>   -
>   -        reader_name__nw_size = reader_name__nelements;
>   -    }
>   -
>   -    nw_size = 0 + reader_name__nw_size;
>   +    nw_size = 12;
>   +    mem_size = sizeof(VSCMsgHeader);
> 
>        /* Check if message fits in reported side */
>        if (nw_size > (uintptr_t) (message_end - start)) {
>   @@ -1986,13 +1980,14 @@ static uint8_t * parse_msgc_smartcard_re
>        if (SPICE_UNLIKELY(data == NULL)) {
>            goto error;
>        }
>   -    end = data + sizeof(VSCMsgReaderAdd);
>   +    end = data + sizeof(VSCMsgHeader);
>        in = start;
> 
>   -    out = (VSCMsgReaderAdd *)data;
>   +    out = (VSCMsgHeader *)data;
> 
>   -    memcpy(out->reader_name, in, reader_name__nelements);
>   -    in += reader_name__nelements;
>   +    out->type = consume_uint32(&in);
>   +    out->reader_id = consume_uint32(&in);
>   +    out->length = consume_uint32(&in);
> 
>        assert(in <= message_end);
>        assert(end <= data + mem_size);
>   @@ -2017,7 +2012,7 @@ static uint8_t * parse_SmartcardChannel_
>            parse_msgc_disconnecting
>        };
>        static parse_msg_func_t funcs2[1] =  {
>   -        parse_msgc_smartcard_reader_add
>   +        parse_msgc_smartcard_header
>        };
>        if (message_type >= 1 && message_type < 7) {
>            return funcs1[message_type-1](message_start, message_end, minor, 
> size_out, free_message);
> 
> Signed-off-by: Frediano Ziglio <fzig...@redhat.com>
> ---
>  spice.proto | 12 ++++++++++--
>  1 file changed, 10 insertions(+), 2 deletions(-)
> 
> Changes since v1:
> - remove spurious changes.
> 
> diff --git a/spice.proto b/spice.proto
> index 4d916bb..69f169e 100644
> --- a/spice.proto
> +++ b/spice.proto
> @@ -1400,6 +1400,12 @@ channel SmartcardChannel : BaseChannel {
>      } @ctype(SpiceMsgSmartcard) data = 101;
>  
>   client:
> +/* Some of the following messages are duplicated, the protocol
> + * definition was broken. Messages, as you can see have same ID.
> + * Also code was not used and didn't compile correctly.
> + * Keeping in the hope could be useful in the future.
> + */
> +/*
>      message {
>       VscMessageHeader header;
>       switch (header.type) {
> @@ -1412,13 +1418,14 @@ channel SmartcardChannel : BaseChannel {
>           VscMessageError error;
>       } u @anon;
>      } @ctype(SpiceMsgcSmartcard) data = 101;
> -
> +*/
>      message {
>       vsc_message_type type;
>       uint32 reader_id;
>       uint32 length;
>      } @ctype(VSCMsgHeader) header = 101;
> -
> +/* See comment on client data message above */
> +/*
>      message {
>       uint32 code;
>      } @ctype(VSCMsgError) error = 101;
> @@ -1430,6 +1437,7 @@ channel SmartcardChannel : BaseChannel {
>      message {
>       int8 reader_name[] @zero_terminated @nonnull;
>      } @ctype(VSCMsgReaderAdd) reader_add = 101;
> +*/
>  } @ifdef(USE_SMARTCARD);
>  
>  channel SpicevmcChannel : BaseChannel {
> -- 
> 2.17.0
> 
> _______________________________________________
> Spice-devel mailing list
> Spice-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/spice-devel

Attachment: signature.asc
Description: PGP signature

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

Reply via email to