[Spice-devel] RedPipeItem lifespan, past, present and ... bug

[Frediano Ziglio]

Hi,
  I recently worked on this area and wanted to share my discovery on the subject.

Some time ago I wrote asking myself what a RedPipeItem is. The answer (to make
short) was an item of a pipe (which is in RedChannelClient).

Basically a RedPipeItem can be added to this pipe while can also, in different
states, be detached from the pipe.

How is managed the life of a RedPipeItem? From RedChannel point of view it's
handled as owner passing (a specific case of single ownership) with exceptions.
Basically:
1- an item is created (external to RedChannel);
2- an item is added to the pipe (calling directly or indirectly client_pipe_add).
   Note: item is passed, client_pipe_add supposes it has the ownership on
   the pointer and callers usually don't use the item anymore;
3- an item is dequeued from the pipe and passed to send_item.
   RedChannelClient won't access the item anymore after calling send_item;
4- send_item will append the item to send_data (RedChannelClient, calling
   red_channel_client_init_send_data) or release the item (in this case
   everything finish here)
5- data will be sent to client;
6- RedChannelClient will release the item (calling release_item callback).

Exceptions:
- RedChannel classes can define a hold_item callback (usually does nothing)
  which is called when data prepared to be sent (step 4 above) which usually
  will increment a reference counter. Currently CursorChannel and DisplayChannel
  implement this function. You will note a call to red_pipe_item_unref in send_item
  of these channels, this to compensate the additional reference, other send_item
  callbacks don't have this function;
- red_channel_client_wait_pipe_item_sent calls hold_item and red_pipe_item_unref.
  This function is supposed to wait until the item is send. It does this keeping
  sending items until the item is detached from the list 
  (ring_item_is_linked(&item->link)) and obviously to avoid use-after-free has to
  make sure the pointer is valid while sending it;
- before appending a message from a RedCharDevice the reference is incremented
  every time is appended to a RedCharDeviceClient 
  (red_char_device_add_msg_to_client_queue).

Implementation of RedPipeItem:
- has a RingItem to make possible to be attached to a pipe;
- has a counter (added quite recently) to manage holding and exceptions;
- has a function to release the item (when counter reach 0).

If everything is clear (but I know I'm a bad teacher) you should easily find the
regression introduced by this commit
https://cgit.freedesktop.org/spice/spice/commit/?id=d232e92794c74a326d1e48355d257f24a6d497c2

Problems:
- RedPipeItem can be inserted only into a single queue, failing to do so
  will trigger an assert during ring_add (this is actually not an issue as multiple
  clients is not supported);
- red_channel_client_wait_pipe_item_sent can crash if called for a channel not
  implementing hold_item;
- red_channel_client_wait_pipe_item_sent make the item be freed later (not a big issue);
- it's easy to forget appending an item to send_data causing a leak if hold_item
  is empty (and this is not well documented);
- all that reference/not reference is quite complicated. Just to prove this
  take into account the commit above and consider that:
  - A wrote the patch, B fixed some problems while doing review and C did some test and
    acked;
  - A, B and C  (not hard to name them ;) ) are all people in the RedHat spice team
    and all quite confident with spice-server code;
  - regression is still there!

Possible future changes (please comment):
- would be good if red_channel_client_wait_pipe_item_sent could work even
  without the hold_item, one possible implementation is adding a fake RedPipeItem
  which when removed will set a flag causing the loop to exit (better would be
  to remove the function and implement this stuff in another way, there are only
  a single call to make sure there are no pending drawing on a surface);
- remove the RingItem and make possible to add the item to multiple clients, this
  IMHO would make stuff much easier to understand;
- remove hold_item and use always red_pipe_item_ref;
- add another callback to RedPipeItem to send the item to make all send_item
  callbacks really small;
- remove atomic operation on RedPipeItem reference counter, I think was added
  to make sure there was no thread issues but it's just slowing down big servers
  with many cores.

If you you didn't get bored and you are still reading trying an explanation of
the above regression I'll give an hint: CharDevice and multiple clients.

Frediano