[Spice-devel] [PATCH 10/11] Make sure link in RedPipeItem can be not the first field
Jonathon Jongsma
jjongsma at redhat.com
Fri May 20 20:44:06 UTC 2016
In general, this is a good idea, but I question why you're doing it now. There
are patches already in the refactory branch which change the PipeItem ring into
a GList (although perhaps GQueue might be more appropriate). That patch would
also fix the underlying issue you're solving here. So having both patches just
creates more work for no benefit.
Jonathon
On Fri, 2016-05-20 at 14:01 +0100, Frediano Ziglio wrote:
> There are many line of code were this assumption is in place.
> For instance RedPipeItem* is converted to RingItem* or
> viceversa.
> Use SPICE_CONTAINEROF to make sure link is in RedPipeItem and the
> offset is computed correctly.
> Also check that RingItem pointer is checked before SPICE_CONTAINEROF.
> On the other direction use &item->link instead of (RingItem*)item.
> This will also make sure that code won't compile if link is
> removed from RedPipeItem.
>
> Signed-off-by: Frediano Ziglio <fziglio at redhat.com>
> ---
> server/dcc-send.c | 23 ++++++++++++-----------
> server/dcc.c | 17 ++++++++++-------
> server/red-channel.c | 9 ++++++---
> server/red-pipe-item.h | 2 ++
> server/reds.c | 3 ++-
> 5 files changed, 32 insertions(+), 22 deletions(-)
>
> diff --git a/server/dcc-send.c b/server/dcc-send.c
> index 8ec22c8..44b8448 100644
> --- a/server/dcc-send.c
> +++ b/server/dcc-send.c
> @@ -188,7 +188,11 @@ static int is_brush_lossy(RedChannelClient *rcc,
> SpiceBrush *brush,
>
> static RedPipeItem *dcc_get_tail(DisplayChannelClient *dcc)
> {
> - return (RedPipeItem*)ring_get_tail(&RED_CHANNEL_CLIENT(dcc)->pipe);
> + RingItem *ring_item = ring_get_tail(&RED_CHANNEL_CLIENT(dcc)->pipe);
> + if (!ring_item) {
> + return NULL;
> + }
> + return SPICE_CONTAINEROF(ring_item, RedPipeItem, link);
> }
>
> static void red_display_add_image_to_pixmap_cache(RedChannelClient *rcc,
> @@ -596,16 +600,14 @@ static int
> pipe_rendered_drawables_intersect_with_areas(DisplayChannelClient *dc
> SpiceRect
> *surface_areas[],
> int num_surfaces)
> {
> - RedPipeItem *pipe_item;
> + RingItem *ring_item;
> Ring *pipe;
>
> spice_assert(num_surfaces);
> pipe = &RED_CHANNEL_CLIENT(dcc)->pipe;
>
> - for (pipe_item = (RedPipeItem *)ring_get_head(pipe);
> - pipe_item;
> - pipe_item = (RedPipeItem *)ring_next(pipe, &pipe_item->link))
> - {
> + RING_FOREACH(ring_item, pipe) {
> + RedPipeItem *pipe_item = SPICE_CONTAINEROF(ring_item, RedPipeItem,
> link);
> Drawable *drawable;
>
> if (pipe_item->type != RED_PIPE_ITEM_TYPE_DRAW)
> @@ -685,7 +687,7 @@ static void
> red_pipe_replace_rendered_drawables_with_images(DisplayChannelClient
> int resent_surface_ids[MAX_PIPE_SIZE];
> SpiceRect resent_areas[MAX_PIPE_SIZE]; // not pointers since drawables
> may be released
> int num_resent;
> - RedPipeItem *pipe_item;
> + RingItem *ring_item;
> Ring *pipe;
>
> resent_surface_ids[0] = first_surface_id;
> @@ -695,9 +697,8 @@ static void
> red_pipe_replace_rendered_drawables_with_images(DisplayChannelClient
> pipe = &RED_CHANNEL_CLIENT(dcc)->pipe;
>
> // going from the oldest to the newest
> - for (pipe_item = (RedPipeItem *)ring_get_tail(pipe);
> - pipe_item;
> - pipe_item = (RedPipeItem *)ring_prev(pipe, &pipe_item->link)) {
> + RING_FOREACH(ring_item, pipe) {
> + RedPipeItem *pipe_item = SPICE_CONTAINEROF(ring_item, RedPipeItem,
> link);
> Drawable *drawable;
> RedDrawablePipeItem *dpi;
> RedImageItem *image;
> @@ -728,7 +729,7 @@ static void
> red_pipe_replace_rendered_drawables_with_images(DisplayChannelClient
>
> spice_assert(image);
> red_channel_client_pipe_remove_and_release(RED_CHANNEL_CLIENT(dcc),
> &dpi->dpi_pipe_item);
> - pipe_item = &image->base;
> + ring_item = &image->base.link;
> }
> }
>
> diff --git a/server/dcc.c b/server/dcc.c
> index 20a8f11..df5123b 100644
> --- a/server/dcc.c
> +++ b/server/dcc.c
> @@ -67,6 +67,7 @@ int
> dcc_clear_surface_drawables_from_pipe(DisplayChannelClient *dcc, int surface
> int wait_if_used)
> {
> Ring *ring;
> + RingItem *ring_item;
> RedPipeItem *item;
> int x;
> RedChannelClient *rcc;
> @@ -76,13 +77,15 @@ int
> dcc_clear_surface_drawables_from_pipe(DisplayChannelClient *dcc, int surface
> no other drawable depends on them */
>
> rcc = RED_CHANNEL_CLIENT(dcc);
> - ring = &rcc->pipe;
> - item = (RedPipeItem *) ring;
> - while ((item = (RedPipeItem *)ring_next(ring, &item->link))) {
> + ring_item = ring = &rcc->pipe;
> + item = NULL;
> + while ((ring_item = ring_next(ring, ring_item))) {
> Drawable *drawable;
> RedDrawablePipeItem *dpi = NULL;
> int depend_found = FALSE;
>
> + item = SPICE_CONTAINEROF(ring_item, RedPipeItem, link);
> +
> if (item->type == RED_PIPE_ITEM_TYPE_DRAW) {
> dpi = SPICE_CONTAINEROF(item, RedDrawablePipeItem,
> dpi_pipe_item);
> drawable = dpi->drawable;
> @@ -94,10 +97,10 @@ int
> dcc_clear_surface_drawables_from_pipe(DisplayChannelClient *dcc, int surface
>
> if (drawable->surface_id == surface_id) {
> RedPipeItem *tmp_item = item;
> - item = (RedPipeItem *)ring_prev(ring, &item->link);
> + ring_item = ring_prev(ring, &item->link);
> red_channel_client_pipe_remove_and_release(rcc, tmp_item);
> - if (!item) {
> - item = (RedPipeItem *)ring;
> + if (!ring_item) {
> + ring_item = ring;
> }
> continue;
> }
> @@ -123,7 +126,7 @@ int
> dcc_clear_surface_drawables_from_pipe(DisplayChannelClient *dcc, int surface
> return TRUE;
> }
>
> - if (item) {
> + if (ring_item) {
> return
> red_channel_client_wait_pipe_item_sent(RED_CHANNEL_CLIENT(dcc), item,
> COMMON_CLIENT_TIMEOUT);
> } else {
> diff --git a/server/red-channel.c b/server/red-channel.c
> index c422afd..20414f3 100644
> --- a/server/red-channel.c
> +++ b/server/red-channel.c
> @@ -1328,13 +1328,15 @@ static inline int
> red_channel_client_waiting_for_ack(RedChannelClient *rcc)
>
> static inline RedPipeItem *red_channel_client_pipe_item_get(RedChannelClient
> *rcc)
> {
> + RingItem *ring_item;
> RedPipeItem *item;
>
> if (!rcc || rcc->send_data.blocked
> || red_channel_client_waiting_for_ack(rcc)
> - || !(item = (RedPipeItem *)ring_get_tail(&rcc->pipe))) {
> + || !(ring_item = ring_get_tail(&rcc->pipe))) {
> return NULL;
> }
> + item = SPICE_CONTAINEROF(ring_item, RedPipeItem, link);
> red_channel_client_pipe_remove(rcc, item);
> return item;
> }
> @@ -1781,12 +1783,13 @@ static void
> red_channel_client_clear_sent_item(RedChannelClient *rcc)
>
> void red_channel_client_pipe_clear(RedChannelClient *rcc)
> {
> - RedPipeItem *item;
> + RingItem *ring_item;
>
> if (rcc) {
> red_channel_client_clear_sent_item(rcc);
> }
> - while ((item = (RedPipeItem *)ring_get_head(&rcc->pipe))) {
> + while ((ring_item = ring_get_head(&rcc->pipe))) {
> + RedPipeItem *item = SPICE_CONTAINEROF(ring_item, RedPipeItem, link);
> ring_remove(&item->link);
> red_pipe_item_unref(item);
> }
> diff --git a/server/red-pipe-item.h b/server/red-pipe-item.h
> index bf13b0b..4856a79 100644
> --- a/server/red-pipe-item.h
> +++ b/server/red-pipe-item.h
> @@ -26,7 +26,9 @@ struct RedPipeItem;
> typedef void red_pipe_item_free_t(struct RedPipeItem *item);
>
> typedef struct RedPipeItem {
> + /* this MUST be the first field, there are many conversion to RingItem*
> */
> RingItem link;
> +
> int type;
>
> /* private */
> diff --git a/server/reds.c b/server/reds.c
> index 90911b4..c983016 100644
> --- a/server/reds.c
> +++ b/server/reds.c
> @@ -747,7 +747,8 @@ static void reds_agent_remove(RedsState *reds)
>
> static void vdi_port_read_buf_release(uint8_t *data, void *opaque)
> {
> - red_pipe_item_unref((RedPipeItem *)opaque);
> + RedVDIReadBuf *read_buf = (RedVDIReadBuf *)opaque;
> + red_pipe_item_unref(&read_buf->base);
> }
>
> /* returns TRUE if the buffer can be forwarded */
More information about the Spice-devel
mailing list