[Spice-devel] [PATCH 06/13] spicevmc: Move SpiceVmcState::pipe_item to RedCharDeviceSpiceVmc

[Jonathon Jongsma]

On Wed, 2016-03-23 at 12:48 +0000, Frediano Ziglio wrote:
> From: Christophe Fergeau <cfergeau at redhat.com>
> 
> This pipe item belongs to the char device, not to the spicevmc channel.

I'm not so sure about this. In general PipeItem seems to be pretty closely tied
to spice communication (i.e. spice channels and channel clients). I'm curious
why you think it belongs in the char device. It may make sense to have the
*buffer* owned by the device, but I'm not sure that the whole pipe item really
belongs in the device. I could be convinced though.

It seems that the only reason that this pipe item is even stored inside either
of these objects is so that we can avoid freeing it if the sif->read() call
returns 0. In that case, we simply cache the newly-allocated PipeItem in the
object (instead of returning it from the function) so that we can re-use it next
time.

On a somewhat-related note: can we add a FIXME to change the type name from
SpiceVmcState to SpiceVmcChannel? I find the current name rather confusing. I
was going to write a quick patch to do that, but I really don't want to
complicate the future patches in the series.

> ---
>  server/spicevmc.c | 13 ++++++-------
>  1 file changed, 6 insertions(+), 7 deletions(-)
> 
> diff --git a/server/spicevmc.c b/server/spicevmc.c
> index 0cf2cce..9a009b9 100644
> --- a/server/spicevmc.c
> +++ b/server/spicevmc.c
> @@ -56,7 +56,6 @@ typedef struct SpiceVmcState {
>      RedChannelClient *rcc;
>      SpiceCharDeviceState *chardev_st;
>      SpiceCharDeviceInstance *chardev_sin;
> -    SpiceVmcPipeItem *pipe_item;
>      SpiceCharDeviceWriteBuffer *recv_from_client_buf;
>      uint8_t port_opened;
>  } SpiceVmcState;
> @@ -143,6 +142,7 @@ static SpiceCharDeviceMsgToClient
> *spicevmc_chardev_read_msg_from_dev(SpiceCharD
>                                                                        void
> *opaque)
>  {
>      SpiceVmcState *state = opaque;
> +    RedCharDeviceSpiceVmc *dev = RED_CHAR_DEVICE_SPICEVMC(sin->st);
>      SpiceCharDeviceInterface *sif;
>      SpiceVmcPipeItem *msg_item;
>      int n;
> @@ -153,14 +153,14 @@ static SpiceCharDeviceMsgToClient
> *spicevmc_chardev_read_msg_from_dev(SpiceCharD
>          return NULL;
>      }
>  
> -    if (!state->pipe_item) {
> +    if (!dev->priv->pipe_item) {
>          msg_item = spice_new0(SpiceVmcPipeItem, 1);
>          msg_item->refs = 1;
>          pipe_item_init(&msg_item->base, PIPE_ITEM_TYPE_SPICEVMC_DATA);
>      } else {
> -        spice_assert(state->pipe_item->buf_used == 0);
> -        msg_item = state->pipe_item;
> -        state->pipe_item = NULL;
> +        spice_assert(dev->priv->pipe_item->buf_used == 0);
> +        msg_item = dev->priv->pipe_item;
> +        dev->priv->pipe_item = NULL;
>      }
>  
>      n = sif->read(sin, msg_item->buf,
> @@ -170,7 +170,7 @@ static SpiceCharDeviceMsgToClient
> *spicevmc_chardev_read_msg_from_dev(SpiceCharD
>          msg_item->buf_used = n;
>          return msg_item;
>      } else {
> -        state->pipe_item = msg_item;
> +        dev->priv->pipe_item = msg_item;
>          return NULL;
>      }
>  }
> @@ -586,7 +586,6 @@ void spicevmc_device_disconnect(RedsState *reds,
> SpiceCharDeviceInstance *sin)
>      sin->st = NULL;
>  
>      reds_unregister_channel(reds, &state->channel);
> -    free(state->pipe_item);
>      red_channel_destroy(&state->channel);
>  }
>