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 _______________________________________________ Spice-devel mailing list Spice-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/spice-devel