[Spice-devel] [server PATCH 0/8] use a SAFE version of RING_FOREACH (worker/channel)

[Christophe Fergeau]

ACK series

Christophe

On Mon, Jul 08, 2013 at 01:32:22PM +0300, Uri Lublin wrote:
> If in RING_FOREACH (in == somewhere in the callee-tree) there is
> a call to read or write, it is not safe, and RING_FOREACH_SAFE
> should be used.
> 
> A call to such read/write that fails, may lead
> to disconnecting from the client, which means
> cleaning up things, and possibly removing from the ring
> the specific node (channel/client) that is currently
> being iterated by the FOREACH loop.
> 
> When the FOREACH loop tries to get the next item, spice-server
> asserts and aborts in ring_next():
>  -- assertion `pos->next != NULL && pos->prev != NULL' failed
> 
> 
> For example (first analysis done by Yonit):
> In red_worker.c (WORKER_FOREACH_DCC):
>   red_pipes_add_drawable
>     red_pipe_add_drawable
>       red_handle_drawable_surfaces_client_synced
>         red_push_surface_image
>           red_channel_client_push
>             red_channel_client_send
>               red_peer_handle_outgoing
>                 reds_stream_writev (if fails -- EPIPE)
>                   handler->cb->on_error = red_channel_client_disconnect()
>                     red_channel_remove_client()
>                       ring_remove() -- of rcc from channel.clients ring.
> 
> 
> In red_channel.c:
> Commit 53488f0275d6c8a121af49f7ac817d09ce68090d fixes two cases of
> such FOREACH loops.
> 
> Since the additional cost of a SAFE loop is insignificant, I replaced
> all such FOREACH macros in red_worker.
> 
> In most pacthes, of red_worker, a FOREACH macro is modified to be safe.
> In the last patch of red_worker, a generic SAFE_FOREACH is introduced.
> 
> If people prefer, I can reorder the patches such that generic macro is
> introduced in the first patch, and the other patches to use it.
> 
> Last, the FOREACH macros can be used outside of red_worker.c.
> Maybe in the future we can move it to an h-file and use it e.g. in red_worker.c
> 
> 
> Uri Lublin (8):
>   red_worker: use only RCC_FOREACH_SAFE
>   red_worker: reuse DCC_FOREACH in WORKER_DCC_FOREACH
>   red_worker: make WORKER_FOREACH_DCC safe
>   red_worker: use only DRAWABLE_FOREACH_GLZ_SAFE
>   red_worker: make DRAWABLE_FOREACH_DPI safe
>   red_worker: make CCC_FOREACH safe
>   red_worker: use a generic SAFE_FOREACH macro
>   red_channel: replace RING_FOREACH with RING_FOREACH_SAFE in some
>     places
> 
>  server/red_channel.c |    8 +-
>  server/red_worker.c  |  190 +++++++++++++++++++++++---------------------------
>  2 files changed, 90 insertions(+), 108 deletions(-)
> 
> _______________________________________________
> Spice-devel mailing list
> Spice-devel at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/spice-devel
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 198 bytes
Desc: not available
URL: <http://lists.freedesktop.org/archives/spice-devel/attachments/20130708/3356e2a6/attachment.pgp>