On Tue, Apr 26, 2016 at 10:44:54AM +0200, Victor Toso wrote:
> Hi,
>
> On Sun, Apr 10, 2016 at 05:24:23PM +0300, Snir Sheriber wrote:
> > 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
> >
> > If SPICE_SPICEVMC_CAP_DATA_COMPRESS_LZ4 capability is available &&
> > data_size > COMPRESS_THRESHOLD data will be sent compressed otherwise
> > data will be sent uncompressed (as well if compression has failed)
> > ---
> > src/channel-usbredir.c | 111 ++++++++++++++++++++++++++++++++++++++++++++++---
> > 1 file changed, 105 insertions(+), 6 deletions(-)
> >
> > diff --git a/src/channel-usbredir.c b/src/channel-usbredir.c
> > index c8a2da9..2b80fd2 100644
> > --- a/src/channel-usbredir.c
> > +++ b/src/channel-usbredir.c
> > @@ -24,6 +24,9 @@
> > #ifdef USE_USBREDIR
> > #include <glib/gi18n.h>
> > #include <usbredirhost.h>
> > +#ifdef USE_LZ4
> > +#include <lz4.h>
> > +#endif
> > #ifdef USE_POLKIT
> > #include "usb-acl-helper.h"
> > #endif
> > @@ -51,6 +54,7 @@
> >
> > #ifdef USE_USBREDIR
> >
> > +#define COMPRESS_THRESHOLD 1000
> > #define SPICE_USBREDIR_CHANNEL_GET_PRIVATE(obj) \
> > (G_TYPE_INSTANCE_GET_PRIVATE((obj), SPICE_TYPE_USBREDIR_CHANNEL, SpiceUsbredirChannelPrivate))
> >
> > @@ -233,6 +237,8 @@ static void channel_set_handlers(SpiceChannelClass *klass)
> > {
> > static const spice_msg_handler handlers[] = {
> > [ SPICE_MSG_SPICEVMC_DATA ] = usbredir_handle_msg,
> > + [ SPICE_MSG_SPICEVMC_COMPRESSED_DATA ] = usbredir_handle_msg,
> > +
> > };
> >
> > spice_channel_set_handlers(klass, handlers, G_N_ELEMENTS(handlers));
> > @@ -269,6 +275,9 @@ void spice_usbredir_channel_set_context(SpiceUsbredirChannel *channel,
> > #if USBREDIR_VERSION >= 0x000701
> > usbredirhost_set_buffered_output_size_cb(priv->host, usbredir_buffered_output_size_callback);
> > #endif
> > +#ifdef USE_LZ4
> > + spice_channel_set_capability(channel, SPICE_SPICEVMC_CAP_DATA_COMPRESS_LZ4);
> > +#endif
> > }
> >
> > static gboolean spice_usbredir_channel_open_device(
> > @@ -632,11 +641,61 @@ static void usbredir_free_write_cb_data(uint8_t *data, void *user_data)
> > usbredirhost_free_write_buffer(priv->host, data);
> > }
> >
> > +#ifdef USE_LZ4
> > +static void usbredir_free_write_cb_compressed_data(uint8_t *data, void *user_data)
> > +{
> > + SpiceUsbredirChannel *channel = user_data;
> > + SpiceUsbredirChannelPrivate *priv = channel->priv;
> > +
> > + usbredirhost_free_write_buffer(priv->host, (uint8_t*)SPICE_CONTAINEROF(data, SpiceMsgCompressedData, compressed_data));
>
> Please break the line here or move SPICE_CONTAINEROF outside the function
>
> > +}
> > +
> > +static int try_write_compress_LZ4(SpiceUsbredirChannel *channel, uint8_t *data, int count) {
> > + SpiceMsgOut *msg_out_compressed;
> > + int bound;
> > +
> > + if(count > COMPRESS_THRESHOLD &&
> > + spice_channel_test_capability(SPICE_CHANNEL(channel),
> > + SPICE_SPICEVMC_CAP_DATA_COMPRESS_LZ4) &&
> > + ((bound = LZ4_compressBound(count)) != 0)) {
>
> We use 4 spaces identation.
>
> > + int compressed_data_count;
> > + SpiceMsgCompressedData *compressed_data_msg;
> > +
> > + compressed_data_msg = (SpiceMsgCompressedData*)spice_malloc(sizeof(SpiceMsgCompressedData) + bound);
> > + compressed_data_msg->uncompressed_size = count;
> > + compressed_data_msg->type = SPICE_DATA_COMPRESSION_TYPE_LZ4;
> > + compressed_data_count = LZ4_compress_default((char*)data,
> > + (char*)compressed_data_msg->compressed_data, count, bound);
> > + if (compressed_data_count > 0) {
> > + compressed_data_msg->compressed_size = compressed_data_count;
> > + msg_out_compressed = spice_msg_out_new(SPICE_CHANNEL(channel),
> > + SPICE_MSGC_SPICEVMC_COMPRESSED_DATA);
> > + msg_out_compressed->marshallers->msg_SpiceMsgCompressedData(msg_out_compressed->marshaller, compressed_data_msg);
> > + spice_marshaller_add_ref_full(msg_out_compressed->marshaller,
> > + (uint8_t*)compressed_data_msg->compressed_data,
> > + compressed_data_count,
> > + usbredir_free_write_cb_compressed_data,
> > + channel);
> > + spice_msg_out_send(msg_out_compressed);
> > + return TRUE;
> > + } else {/* free & fallback to sending the message uncompressed */
> > + free(compressed_data_msg);
> > + return FALSE;
> > + }
> > + }
> > + return FALSE;
>
> I would prefer early return, like:
>
> instead of:
>
> if (count > COMPRESS_THRESHOLD &&
> spice_channel_test_capability(SPICE_CHANNEL(channel), SPICE_SPICEVMC_CAP_DATA_COMPRESS_LZ4) &&
> ((bound = LZ4_compressBound(count)) != 0)) {
> ...
> }
> return FALSE;
>
> to
>
> if (count <= COMPRESS_THRESHOLD) {
> /* Not enough data to compress */
> return FALSE;
> }
>
> bound = LZ4_compressBound (count));
> if (!spice_channel_test_capability(SPICE_CHANNEL(channel), SPICE_SPICEVMC_CAP_DATA_COMPRESS_LZ4) ||
> bound == 0) {
> /* We will not compress data */
> return FALSE;
> ...
> }
> > +}
> > +#endif
> > +
> > static int usbredir_write_callback(void *user_data, uint8_t *data, int count)
> > {
> > SpiceUsbredirChannel *channel = user_data;
> > SpiceMsgOut *msg_out;
> >
> > +#ifdef USE_LZ4
> > + if(try_write_compress_LZ4(channel, data, count))
> > + return count;
> > +#endif
> > msg_out = spice_msg_out_new(SPICE_CHANNEL(channel),
> > SPICE_MSGC_SPICEVMC_DATA);
> > spice_marshaller_add_ref_full(msg_out->marshaller, data, count,
> > @@ -724,11 +783,41 @@ static void spice_usbredir_channel_up(SpiceChannel *c)
> > usbredirhost_write_guest_data(priv->host);
> > }
> >
> > +static int try_handle_compressed_msg(SpiceMsgCompressedData *compressed_data_msg, uint8_t **buf, int *size) {
> > + uint32_t decompressed_size = 0;
> > + char* decompressed;
>
> Better start *decompressed with NULL here;
> (also, * stays with the var, not with the type);
>
> > +
> > + switch(compressed_data_msg->type) {
> > +#ifdef USE_LZ4
> > + case SPICE_DATA_COMPRESSION_TYPE_LZ4:
> > + decompressed = (char*)malloc(compressed_data_msg->uncompressed_size);
>
> Please use g_malloc and you don't need to cast here
>
> > + decompressed_size = LZ4_decompress_safe ((char*)compressed_data_msg->compressed_data,
> > + decompressed,
> > + compressed_data_msg->compressed_size,
> > + compressed_data_msg->uncompressed_size);
>
> if LZ4_decompress_safe fails, you will be leaking decompressed;
>
> > + break;
> > +#endif
> > + default:
> > + decompressed = NULL;
> > + g_warning("Unknown Compression Type");
> > + return FALSE;
> > + }
> > + if (decompressed_size < 1 || decompressed_size!= compressed_data_msg->uncompressed_size) {
>
> I guess that the second term of the conditional works for the first term as
> well? You might g_free the decompressed ptr here;
>
> > + g_warning("Decompress Error decompressed_size=%d expected=%d",
> > + decompressed_size, compressed_data_msg->uncompressed_size);
> > + return FALSE;
> > + } else {
>
> You don't need the else here
>
> > + *size = decompressed_size;
> > + *buf = (uint8_t*)decompressed;
> > + return TRUE;
> > + }
> > +}
> > +
> > static void usbredir_handle_msg(SpiceChannel *c, SpiceMsgIn *in)
> > {
> > SpiceUsbredirChannel *channel = SPICE_USBREDIR_CHANNEL(c);
> > SpiceUsbredirChannelPrivate *priv = channel->priv;
> > - int r, size;
> > + int r = 0, size;
> > uint8_t *buf;
> >
> > g_return_if_fail(priv->host != NULL);
> > @@ -736,13 +825,23 @@ static void usbredir_handle_msg(SpiceChannel *c, SpiceMsgIn *in)
> > /* No recursion allowed! */
> > g_return_if_fail(priv->read_buf == NULL);
> >
> > - buf = spice_msg_in_raw(in, &size);
> > - priv->read_buf = buf;
> > - priv->read_buf_size = size;
> > + if (spice_msg_in_type(in) == SPICE_MSG_SPICEVMC_COMPRESSED_DATA) {
> > + SpiceMsgCompressedData *compressed_data_msg = spice_msg_in_parsed(in);
> > + if(try_handle_compressed_msg(compressed_data_msg, &buf, &size)) {
> > + priv->read_buf_size = size;
> > + priv->read_buf = buf;
> > + } else {
> > + r = usbredirhost_read_parse_error;
> > + }
> > + } else { /*Regular SPICE_MSG_SPICEVMC_DATA msg*/
> > + buf = spice_msg_in_raw(in, &size);
> > + priv->read_buf_size = size;
> > + priv->read_buf = buf;
> > + }
> >
> > spice_usbredir_channel_lock(channel);
> > -
> > - r = usbredirhost_read_guest_data(priv->host);
> > + if (r == 0)
> > + r = usbredirhost_read_guest_data(priv->host);
>
> I think you can skip the channel_lock/unlock instead.
>
> if (r != 0)
> return;
Sorry, I misread the code, my suggestion is not good ;)
>
> spice_usbredir_channel_lock(channel);
> r = usbredirhost_read_guest_data(priv->host);
> if (r != 0) {
> ...
> }
> spice_usbredir_channel_unlock(channel);
>
> > if (r != 0) {
> > SpiceUsbDevice *spice_device = priv->spice_device;
> > device_error_data err_data;
> > --
> > 2.5.5
>
> Could you please resend this one, spice-protocol, spice-common together with
> spice-server after you rebase the later? I'll try to test/review it ASAP then.
>
> PS: double check the indentation and if all castings here and spice-server are
> necessary... Also, why not spice_warning instead of g_warning?
>
> Many thanks!
> toso
_______________________________________________
Spice-devel mailing list
Spice-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/spice-devel