[Spice-devel] [spice-common PATCH v3] Add LZ4 data compression and used it in spicevmc channel

[Frediano Ziglio]

> > 
> > Compressed message type is CompressedData which contains compression
> > type (1 byte) followed by the uncompressed data size (4 bytes) followed
> > by the compressed data size (4 bytes) followed by the compressed data
> > ---
> >  common/client_marshallers.h |  1 +
> >  common/messages.h           |  7 +++++++
> >  spice.proto                 | 15 +++++++++++++++
> >  3 files changed, 23 insertions(+)
> > 
> > diff --git a/common/client_marshallers.h b/common/client_marshallers.h
> > index 728987e..2074323 100644
> > --- a/common/client_marshallers.h
> > +++ b/common/client_marshallers.h
> > @@ -33,6 +33,7 @@ SPICE_BEGIN_DECLS
> >  typedef struct {
> >      void (*msg_SpiceMsgEmpty)(SpiceMarshaller *m, SpiceMsgEmpty *msg);
> >      void (*msg_SpiceMsgData)(SpiceMarshaller *m, SpiceMsgData *msg);
> > +    void (*msg_SpiceMsgCompressedData)(SpiceMarshaller *m,
> > SpiceMsgCompressedData *msg);
> >      void (*msgc_ack_sync)(SpiceMarshaller *m, SpiceMsgcAckSync *msg);
> >      void (*msgc_pong)(SpiceMarshaller *m, SpiceMsgPing *msg);
> >      void (*msgc_disconnecting)(SpiceMarshaller *m, SpiceMsgDisconnect
> >      *msg);
> > diff --git a/common/messages.h b/common/messages.h
> > index f537950..d001850 100644
> > --- a/common/messages.h
> > +++ b/common/messages.h
> > @@ -55,6 +55,13 @@ typedef struct SpiceMsgData {
> >      uint8_t data[0];
> >  } SpiceMsgData;
> >  
> > +typedef struct SpiceMsgCompressedData {
> > +    uint8_t type;
> > +    uint32_t uncompressed_size;
> > +    uint32_t compressed_size;
> > +    uint8_t compressed_data[0];
> > +} SpiceMsgCompressedData;
> > +

This would became

typedef struct SpiceMsgCompressedData {
    uint8_t type;
    uint32_t uncompressed_size;
    uint32_t compressed_size;
    uint8_t *compressed_data;
} SpiceMsgCompressedData;

> >  typedef struct SpiceMsgEmpty {
> >      uint8_t padding;
> >  } SpiceMsgEmpty;
> > diff --git a/spice.proto b/spice.proto
> > index 4a0778d..ad210ca 100644
> > --- a/spice.proto
> > +++ b/spice.proto
> > @@ -120,6 +120,19 @@ message Data {
> >      uint8 data[] @end @ctype(uint8_t);
> >  } @nocopy;
> >  
> > +enum8 data_compression_type {
> > +    INVALID,
> > +    NONE,
> > +    LZ4,
> > +};
> > +
> > +message CompressedData {
> > +    data_compression_type type;
> > +    uint32 uncompressed_size;
> > +    uint32 compressed_size;
> > +    uint8 compressed_data[] @end;
> > +} @ctype(SpiceMsgCompressedData);
> > +

This

message CompressedData {
    data_compression_type type;
    uint32 uncompressed_size;
    uint8 compressed_data[] @as_ptr(compressed_size);
} @ctype(SpiceMsgCompressedData);

You could also avoid the uncompressed_size in case of NONE with

message CompressedData {
    data_compression_type type;
    switch (type) {
    case !NONE:
        uint32 uncompressed_size;
    } u @anon;
    uint8 compressed_data[] @as_ptr(compressed_size);
} @ctype(SpiceMsgCompressedData);

security consideration: what happens if the sender send a very
huge (in theory a byte less than 4g) uncompressed_size ?

> 
> Just a consideration. What's the size of compressed_data array?
> Why not using @as_ptr attribute and remove the compressed_size field?
> In this way you'll save 4 bytes for every packet.
> 
> >  struct ChannelWait {
> >      uint8 channel_type;
> >      uint8 channel_id;
> > @@ -1373,8 +1386,10 @@ channel SmartcardChannel : BaseChannel {
> >  channel SpicevmcChannel : BaseChannel {
> >  server:
> >      Data data = 101;
> > +    CompressedData compressed_data = 102;
> >  client:
> >      Data data = 101;
> > +    CompressedData compressed_data = 102;
> >  };
> >  
> >  channel UsbredirChannel : SpicevmcChannel {
> 

Frediano