[Spice-devel] [PATCH 14/20] Use GQueue for RedCharDevice::send_queue

[Frediano Ziglio]

> 
> On Fri, Apr 15, 2016 at 03:01:37PM -0500, Jonathon Jongsma wrote:
> > > @@ -394,21 +376,17 @@ static int
> > > red_char_device_read_from_device(RedCharDevice *dev)
> > >  
> > >  static void red_char_device_client_send_queue_push(RedCharDeviceClient
> > > *dev_client)
> > >  {
> > > -    RingItem *item;
> > > -    while ((item = ring_get_tail(&dev_client->send_queue)) &&
> > > -           red_char_device_can_send_to_client(dev_client)) {
> > > -        RedCharDeviceMsgToClientItem *msg_item;
> > > -
> > > -        msg_item = SPICE_CONTAINEROF(item, RedCharDeviceMsgToClientItem,
> > > link);
> > > -        ring_remove(item);
> > > -
> > > +    while (!g_queue_is_empty(dev_client->send_queue)) {
> > > +        GList *msg;
> > 
> > GList* ?? Doesn't this GQueue contain PipeItem*?
> > 
> > > +        if (!red_char_device_can_send_to_client(dev_client)) {
> > > +            break;
> > > +        }
> > > +        msg = g_queue_pop_tail(dev_client->send_queue);
> > > +        g_assert(msg != NULL);
> > >          dev_client->num_send_tokens--;
> > > -        red_char_device_send_msg_to_client(dev_client->dev,
> > > -                                           msg_item->msg,
> > > +        red_char_device_send_msg_to_client(dev_client->dev, msg->data,
> > 
> > I think the second argument should simply be 'msg' instead of 'msg->data',
> > no? I
> > wonder why it works...
> > 
> > >                                             dev_client->client);
> > > -        pipe_item_unref(msg_item->msg);
> > > -        dev_client->send_queue_size--;
> > > -        free(msg_item);
> > > +        pipe_item_unref(msg);
> > 
> > 
> > ...and here, we free this GList* variable by calling pipe_item_unref().
> > That's
> > one of the pitfalls of making pipe_item_unref() accept a gpointer argument
> > --
> > the compiler didn't warn us...
> 
> Yes, looks like I got this hunk wrong. I must say that for most of these
> changes, I did not go further than trying compilation, I did not
> carefullly test them, so I'm not surprised to find such bugs :(
> Might be better to delay merging some of these patches until I tested
> them more carefully ;)
> 
> Christophe
> 

At least I use some replay recording. However in this case the path are
bound to the agent so requires a VM.

I'm noting that last smoketest is dated 21 March. Are there problem
with smoketests ?

Frediano