On Thu, 2016-04-07 at 17:11 -0500, Jonathon Jongsma wrote:
> From: Christophe Fergeau <cfergeau@xxxxxxxxxx>
>
> Since RedPipeItem is already refcounted, this allows to remove various
> layers of ref/unref helpers from reds.c, and use the generic
> red_pipe_item_{ref, unref} instead.
Hmm, I don't know if I like making this a RedPipeItem just to get the
refcounting. It seems like it would be better to simply return an actual pipe
item (e.g. AgentDataPipeItem) rather than VDIReadBuf from
read_one_msg_from_device(). That would match the other char device
implementations more closely as well...
> ---
> server/reds.c | 67 +++++++++++++++++++++++++++-------------------------------
> -
> 1 file changed, 31 insertions(+), 36 deletions(-)
>
> diff --git a/server/reds.c b/server/reds.c
> index 1c37627..64f7df8 100644
> --- a/server/reds.c
> +++ b/server/reds.c
> @@ -167,8 +167,7 @@ struct ChannelSecurityOptions {
> };
>
> typedef struct VDIReadBuf {
> - RingItem link;
> - uint32_t refs;
> + RedPipeItem link;
If we do take this approach, I don't think that this should still be named
'link'. That's rather confusing.
Reviewed-by: Jonathon Jongsma <jjongsma@xxxxxxxxxx>
> RedCharDeviceVDIPort *dev;
>
> int len;
> @@ -265,8 +264,6 @@ static uint32_t reds_qxl_ram_size(RedsState *reds);
> static int calc_compression_level(RedsState *reds);
>
> static VDIReadBuf *vdi_port_get_read_buf(RedCharDeviceVDIPort *dev);
> -static VDIReadBuf *vdi_port_read_buf_ref(VDIReadBuf *buf);
> -static void vdi_port_read_buf_unref(VDIReadBuf *buf);
>
> static ChannelSecurityOptions *reds_find_channel_security(RedsState *reds,
> int id)
> {
> @@ -486,7 +483,7 @@ static void reds_reset_vdp(RedsState *reds)
> dev->priv->receive_len = sizeof(dev->priv->vdi_chunk_header);
> dev->priv->message_receive_len = 0;
> if (dev->priv->current_read_buf) {
> - vdi_port_read_buf_unref(dev->priv->current_read_buf);
> + red_pipe_item_unref(dev->priv->current_read_buf);
> dev->priv->current_read_buf = NULL;
> }
> /* Reset read filter to start with clean state when the agent reconnects
> */
> @@ -705,15 +702,11 @@ static void reds_agent_remove(RedsState *reds)
> }
> }
>
> -/*******************************
> - * Char device state callbacks *
> - * *****************************/
> -
> static void vdi_port_read_buf_release(uint8_t *data, void *opaque)
> {
> VDIReadBuf *buf = (VDIReadBuf *)opaque;
>
> - vdi_port_read_buf_unref(buf);
> + red_pipe_item_unref(buf);
> }
>
> /* returns TRUE if the buffer can be forwarded */
> @@ -756,32 +749,25 @@ static VDIReadBuf
> *vdi_port_get_read_buf(RedCharDeviceVDIPort *dev)
> }
>
> ring_remove(item);
> - buf = SPICE_CONTAINEROF(item, VDIReadBuf, link);
> + buf = SPICE_CONTAINEROF(item, VDIReadBuf, link.parent.link);
>
> - buf->refs = 1;
> - return buf;
> -}
> + g_warn_if_fail(buf->link.refcount == 0);
> + red_pipe_item_ref(buf);
>
> -static VDIReadBuf* vdi_port_read_buf_ref(VDIReadBuf *buf)
> -{
> - buf->refs++;
> return buf;
> }
>
> -/* FIXME: refactor so that unreffing the VDIReadBuf doesn't require accessing
> - * RedsState? */
> -static void vdi_port_read_buf_unref(VDIReadBuf *buf)
> +static void vdi_port_read_buf_free(VDIReadBuf *buf)
> {
> - if (!--buf->refs) {
> - ring_add(&buf->dev->priv->read_bufs, &buf->link);
> + g_warn_if_fail(buf->link.refcount == 0);
> + ring_add(&buf->dev->priv->read_bufs, (RingItem *)buf);
>
> - /* read_one_msg_from_vdi_port may have never completed because the
> read_bufs
> - ring was empty. So we call it again so it can complete its work if
> - necessary. Note that since we can be called from
> red_char_device_wakeup
> - this can cause recursion, but we have protection for that */
> - if (buf->dev->priv->agent_attached) {
> - red_char_device_wakeup(RED_CHAR_DEVICE(buf->dev));
> - }
> + /* read_one_msg_from_vdi_port may have never completed because the
> read_bufs
> + ring was empty. So we call it again so it can complete its work if
> + necessary. Note that since we can be called from
> red_char_device_wakeup
> + this can cause recursion, but we have protection for that */
> + if (buf->dev->priv->agent_attached) {
> + red_char_device_wakeup(RED_CHAR_DEVICE(buf->dev));
> }
> }
>
> @@ -851,7 +837,7 @@ static RedCharDeviceMsgToClient
> *vdi_port_read_one_msg_from_device(SpiceCharDevi
> if (error) {
> reds_agent_remove(reds);
> }
> - vdi_port_read_buf_unref(dispatch_buf);
> + red_pipe_item_unref(dispatch_buf);
> }
> }
> } /* END switch */
> @@ -866,11 +852,12 @@ static void
> vdi_port_send_msg_to_client(RedCharDeviceMsgToClient *msg,
> {
> VDIReadBuf *agent_data_buf = msg;
>
> + red_pipe_item_ref(agent_data_buf);
> main_channel_client_push_agent_data(red_client_get_main(client),
> agent_data_buf->data,
> agent_data_buf->len,
> vdi_port_read_buf_release,
> -
> vdi_port_read_buf_ref(agent_data_buf));
> + agent_data_buf);
> }
>
> static void vdi_port_send_tokens_to_client(RedClient *client, uint32_t
> tokens, void *opaque)
> @@ -1220,7 +1207,7 @@ void reds_on_main_channel_migrate(RedsState *reds,
> MainChannelClient *mcc)
> if (error) {
> reds_agent_remove(reds);
> }
> - vdi_port_read_buf_unref(read_buf);
> + red_pipe_item_unref(read_buf);
> }
>
> spice_assert(agent_dev->priv->receive_len);
> @@ -4292,8 +4279,16 @@ red_char_device_vdi_port_init(RedCharDeviceVDIPort
> *self)
> for (i = 0; i < REDS_VDI_PORT_NUM_RECEIVE_BUFFS; i++) {
> VDIReadBuf *buf = spice_new0(VDIReadBuf, 1);
> buf->dev = self;
> - ring_item_init(&buf->link);
> - ring_add(&self->priv->read_bufs, &buf->link);
> + /* Bogus pipe item type, we only need the RingItem and refcounting
> + * from the base class and are not going to use the type
> + */
> + red_pipe_item_init(&buf->link, -1,
> + (GDestroyNotify)vdi_port_read_buf_free);
> + g_warn_if_fail(!self->priv->agent_attached);
> + /* This ensures the newly created buffer is placed in the
> + * RedCharDeviceVDIPort::read_bufs queue ready to be reused
> + */
> + red_pipe_item_unref(buf);
> }
> }
>
> @@ -4318,8 +4313,8 @@
> red_char_device_vdi_port_class_init(RedCharDeviceVDIPortClass *klass)
> object_class->constructed = red_char_device_vdi_port_constructed;
>
> char_dev_class->read_one_msg_from_device =
> vdi_port_read_one_msg_from_device;
> - char_dev_class->ref_msg_to_client =
> (RedCharDeviceRefMsgToClient)vdi_port_read_buf_ref;
> - char_dev_class->unref_msg_to_client =
> (RedCharDeviceUnrefMsgToClient)vdi_port_read_buf_unref;
> + char_dev_class->ref_msg_to_client =
> (RedCharDeviceRefMsgToClient)red_pipe_item_ref;
> + char_dev_class->unref_msg_to_client =
> (RedCharDeviceUnrefMsgToClient)red_pipe_item_unref;
> char_dev_class->send_msg_to_client = vdi_port_send_msg_to_client;
> char_dev_class->send_tokens_to_client = vdi_port_send_tokens_to_client;
> char_dev_class->remove_client = vdi_port_remove_client;
_______________________________________________
Spice-devel mailing list
Spice-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/spice-devel