[Spice-devel] [PATCH spice-server v2 1/2] spicevmc: Channel is owned by device
Frediano Ziglio
fziglio at redhat.com
Fri Nov 4 15:24:18 UTC 2016
>
> On Fri, 2016-11-04 at 07:56 -0400, Frediano Ziglio wrote:
> > The subject is a bit confusing to me.
> > Looks like is nor a "what" not a "why", just a statement.
> >
> > >
> > >
> > > From: Jonathon Jongsma <jjongsma at redhat.com>
> > >
> > > Previously, spicevmc_device_connect() created a channel, which then
> > > internally created a device. Then we returned the internal device
> > > from
> > > the channel to the caller. The channel essentially owned the
> > > device, but
> > > it makes more sense for the device to own the channel, because a
> > > channel
> > > can't really exist without the associated device.
> > >
> >
> > This is misleading, in the current master code device owns the
> > channel
> > while it seems that with this patch you are reverting the order but
> > instead
> > you are reverting the creation order.
> >
> > >
> > > So this refactory inverts things: spicevmc_device_connect() now
> > > creates
> > > a char device and returns that directly. Internally, that device
> > > creates
> > > an associated channel.
> > > ---
> > > server/spicevmc.c | 122
> > > +++++++++++++++++++++++++++++++++++++-----------------
> > > 1 file changed, 83 insertions(+), 39 deletions(-)
> > >
> > > diff --git a/server/spicevmc.c b/server/spicevmc.c
> > > index 0fcdc96..ba6e358 100644
> > > --- a/server/spicevmc.c
> > > +++ b/server/spicevmc.c
> > > @@ -78,6 +78,7 @@ typedef struct RedCharDeviceSpiceVmcClass
> > > RedCharDeviceSpiceVmcClass;
> > >
> > > struct RedCharDeviceSpiceVmc {
> > > RedCharDevice parent;
> > > + uint8_t channel_type;
> > > RedVmcChannel *channel;
> > > };
> > >
> > > @@ -89,7 +90,7 @@ struct RedCharDeviceSpiceVmcClass
> > > static GType red_char_device_spicevmc_get_type(void) G_GNUC_CONST;
> > > static RedCharDevice
> > > *red_char_device_spicevmc_new(SpiceCharDeviceInstance
> > > *sin,
> > > RedsState
> > > *reds,
> > > - void *opaque);
> > > + uint8_t
> > > channel_type);
> > >
> > > G_DEFINE_TYPE(RedCharDeviceSpiceVmc, red_char_device_spicevmc,
> > > RED_TYPE_CHAR_DEVICE)
> > >
> > > @@ -109,8 +110,7 @@ struct RedVmcChannel
> > > RedChannel parent;
> > >
> > > RedChannelClient *rcc;
> > > - RedCharDevice *chardev;
> > > - SpiceCharDeviceInstance *chardev_sin;
> > > + RedCharDevice *chardev; /* weak */
> > > RedVmcPipeItem *pipe_item;
> > > RedCharDeviceWriteBuffer *recv_from_client_buf;
> > > uint8_t port_opened;
> > > @@ -180,7 +180,7 @@ G_DEFINE_TYPE(RedVmcChannelPort,
> > > red_vmc_channel_port,
> > > RED_TYPE_VMC_CHANNEL)
> > >
> > > enum {
> > > PROP0,
> > > - PROP_DEVICE_INSTANCE
> > > + PROP_CHAR_DEVICE
> > > };
> > >
> > > static void
> > > @@ -193,8 +193,8 @@ red_vmc_channel_get_property(GObject *object,
> > >
> > > switch (property_id)
> > > {
> > > - case PROP_DEVICE_INSTANCE:
> > > - g_value_set_pointer(value, self->chardev_sin);
> > > + case PROP_CHAR_DEVICE:
> > > + g_value_set_object(value, self->chardev);
> > > break;
> > > default:
> > > G_OBJECT_WARN_INVALID_PROPERTY_ID(object, property_id,
> > > pspec);
> > > @@ -211,8 +211,9 @@ red_vmc_channel_set_property(GObject *object,
> > >
> > > switch (property_id)
> > > {
> > > - case PROP_DEVICE_INSTANCE:
> > > - self->chardev_sin = g_value_get_pointer(value);
> > > + case PROP_CHAR_DEVICE:
> > > + /* don't take a ref */
> > > + self->chardev = g_value_get_object(value);
> > > break;
> > > default:
> > > G_OBJECT_WARN_INVALID_PROPERTY_ID(object, property_id,
> > > pspec);
> > > @@ -241,8 +242,6 @@ red_vmc_channel_constructed(GObject *object)
> > >
> > > red_channel_init_outgoing_messages_window(RED_CHANNEL(self));
> > >
> > > - self->chardev = red_char_device_spicevmc_new(self-
> > > >chardev_sin, reds,
> > > self);
> > > -
> > > reds_register_channel(reds, RED_CHANNEL(self));
> > > }
> > >
> > > @@ -265,7 +264,7 @@ red_vmc_channel_finalize(GObject *object)
> > > }
> > >
> > > static RedVmcChannel *red_vmc_channel_new(RedsState *reds, uint8_t
> > > channel_type,
> > > - SpiceCharDeviceInstance
> > > *sin)
> > > + RedCharDeviceSpiceVmc
> > > *chardev)
> > > {
> > > GType gtype = G_TYPE_NONE;
> > > static uint8_t id[256] = { 0, };
> > > @@ -282,7 +281,9 @@ static RedVmcChannel
> > > *red_vmc_channel_new(RedsState
> > > *reds, uint8_t channel_type,
> > > break;
> > > default:
> > > g_error("Unsupported channel_type for
> > > red_vmc_channel_new():
> > > %u", channel_type);
> > > + return NULL;
> > > }
> > > +
> > > return g_object_new(gtype,
> > > "spice-server", reds,
> > > "core-interface",
> > > reds_get_core_interface(reds),
> > > @@ -291,7 +292,7 @@ static RedVmcChannel
> > > *red_vmc_channel_new(RedsState
> > > *reds, uint8_t channel_type,
> > > "handle-acks", FALSE,
> > > "migration-flags",
> > > (SPICE_MIGRATE_NEED_FLUSH |
> > > SPICE_MIGRATE_NEED_DATA_TRANSFER),
> > > - "device-instance", sin,
> > > + "char-device", chardev,
> > > NULL);
> > > }
> > >
> > > @@ -421,9 +422,11 @@ static void
> > > spicevmc_chardev_send_msg_to_client(RedPipeItem *msg,
> > > static void spicevmc_port_send_init(RedChannelClient *rcc)
> > > {
> > > RedVmcChannel *channel =
> > > RED_VMC_CHANNEL(red_channel_client_get_channel(rcc));
> > > - SpiceCharDeviceInstance *sin = channel->chardev_sin;
> > > + SpiceCharDeviceInstance *sin;
> > > RedPortInitPipeItem *item =
> > > spice_malloc(sizeof(RedPortInitPipeItem));
> > >
> > > + g_object_get(channel->chardev, "sin", &sin, NULL);
> > > +
> > > red_pipe_item_init(&item->base, RED_PIPE_ITEM_TYPE_PORT_INIT);
> > > item->name = strdup(sin->portname);
> > > item->opened = channel->port_opened;
> > > @@ -481,6 +484,7 @@ static int
> > > spicevmc_red_channel_client_config_socket(RedChannelClient *rcc)
> > > static void
> > > spicevmc_red_channel_client_on_disconnect(RedChannelClient *rcc)
> > > {
> > > RedVmcChannel *channel;
> > > + SpiceCharDeviceInstance *sin;
> > > SpiceCharDeviceInterface *sif;
> > > RedClient *client = red_channel_client_get_client(rcc);
> > >
> > > @@ -493,13 +497,12 @@ static void
> > > spicevmc_red_channel_client_on_disconnect(RedChannelClient *rcc)
> > > /* partial message which wasn't pushed to device */
> > > red_char_device_write_buffer_release(channel->chardev,
> > > &channel->recv_from_client_buf);
> > >
> > > - if (channel->chardev) {
> > > - if (red_char_device_client_exists(channel->chardev,
> > > client)) {
> > > - red_char_device_client_remove(channel->chardev,
> > > client);
> > > - } else {
> > > - spice_printerr("client %p have already been removed
> > > from char
> > > dev %p",
> > > - client, channel->chardev);
> > > - }
> > > + g_object_get(channel->chardev, "sin", &sin, NULL);
> > > + if (red_char_device_client_exists(channel->chardev, client)) {
> > > + red_char_device_client_remove(channel->chardev, client);
> > > + } else {
> > > + spice_printerr("client %p have already been removed from
> > > char dev
> > > %p",
> > > + client, channel->chardev);
> > > }
> > >
> > > /* Don't destroy the rcc if it is already being destroyed, as
> > > then
> > > @@ -508,9 +511,9 @@ static void
> > > spicevmc_red_channel_client_on_disconnect(RedChannelClient *rcc)
> > > red_channel_client_destroy(rcc);
> > >
> > > channel->rcc = NULL;
> > > - sif = spice_char_device_get_interface(channel->chardev_sin);
> > > + sif = spice_char_device_get_interface(sin);
> > > if (sif->state) {
> > > - sif->state(channel->chardev_sin, 0);
> > > + sif->state(sin, 0);
> > > }
> > > }
> > >
> > > @@ -590,10 +593,12 @@ static int
> > > spicevmc_red_channel_client_handle_message_parsed(RedChannelClient
> > > *r
> > > /* NOTE: *msg free by free() (when cb to
> > > spicevmc_red_channel_release_msg_rcv_buf
> > > * with the compressed msg type) */
> > > RedVmcChannel *channel;
> > > + SpiceCharDeviceInstance *sin;
> > > SpiceCharDeviceInterface *sif;
> > >
> > > channel =
> > > RED_VMC_CHANNEL(red_channel_client_get_channel(rcc));
> > > - sif = spice_char_device_get_interface(channel->chardev_sin);
> > > + g_object_get(channel->chardev, "sin", &sin, NULL);
> > > + sif = spice_char_device_get_interface(sin);
> > >
> > > switch (type) {
> > > case SPICE_MSGC_SPICEVMC_DATA:
> > > @@ -611,7 +616,7 @@ static int
> > > spicevmc_red_channel_client_handle_message_parsed(RedChannelClient
> > > *r
> > > return FALSE;
> > > }
> > > if (sif->base.minor_version >= 2 && sif->event != NULL)
> > > - sif->event(channel->chardev_sin, *(uint8_t*)msg);
> > > + sif->event(sin, *(uint8_t*)msg);
> > > break;
> > > default:
> > > return red_channel_client_handle_message(rcc, size, type,
> > > (uint8_t*)msg);
> > > @@ -776,10 +781,11 @@ red_vmc_channel_class_init(RedVmcChannelClass
> > > *klass)
> > > channel_class->handle_migrate_data =
> > > spicevmc_channel_client_handle_migrate_data;
> > >
> > > g_object_class_install_property(object_class,
> > > - PROP_DEVICE_INSTANCE,
> > > - g_param_spec_pointer("device-
> > > instance",
> > > - "device
> > > instance",
> > > - "Device
> > > instance
> > > for this channel",
> > > + PROP_CHAR_DEVICE,
> > > + g_param_spec_object("char-
> > > device",
> > > + "Char
> > > device",
> > > + "Char
> > > device for
> > > this channel",
> > > +
> > > RED_TYPE_CHAR_DEVICE_SPICEVMC,
> > > G_PARAM_R
> > > EADWRITE |
> > > G_PARAM_C
> > > ONSTRUCT_ONLY
> > > |
> > > G_PARAM_S
> > > TATIC_STRINGS));
> > > @@ -820,8 +826,8 @@ static void spicevmc_connect(RedChannel
> > > *channel,
> > > RedClient *client,
> > > uint32_t type, id;
> > >
> > > vmc_channel = RED_VMC_CHANNEL(channel);
> > > - sin = vmc_channel->chardev_sin;
> > > g_object_get(channel, "channel-type", &type, "id", &id, NULL);
> > > + g_object_get(vmc_channel->chardev, "sin", &sin, NULL);
> > >
> > > if (vmc_channel->rcc) {
> > > spice_printerr("channel client %d:%d (%p) already
> > > connected,
> > > refusing second connection",
> > > @@ -851,7 +857,7 @@ static void spicevmc_connect(RedChannel
> > > *channel,
> > > RedClient *client,
> > > return;
> > > }
> > >
> > > - sif = spice_char_device_get_interface(vmc_channel-
> > > >chardev_sin);
> > > + sif = spice_char_device_get_interface(sin);
> > > if (sif->state) {
> > > sif->state(sin, 1);
> > > }
> > > @@ -861,13 +867,7 @@ RedCharDevice
> > > *spicevmc_device_connect(RedsState *reds,
> > > SpiceCharDeviceInstance
> > > *sin,
> > > uint8_t channel_type)
> > > {
> > > - RedCharDeviceSpiceVmc *dev_state;
> > > - RedVmcChannel *state = red_vmc_channel_new(reds, channel_type,
> > > sin);
> > > -
> > > - dev_state = RED_CHAR_DEVICE_SPICEVMC(state->chardev);
> > > - dev_state->channel = state;
> > > -
> > > - return RED_CHAR_DEVICE(dev_state);
> > > + return red_char_device_spicevmc_new(sin, reds, channel_type);
> > > }
> > >
> > > /* Must be called from RedClient handling thread. */
> > > @@ -922,18 +922,62 @@ red_char_device_spicevmc_dispose(GObject
> > > *object)
> > > }
> > > }
> > >
> > > +enum {
> > > + PROP_CHANNEL_TYPE = 1
> > > +};
> > > +
> > > +static void
> > > +red_char_device_spicevmc_set_property(GObject *object,
> > > + guint property_id,
> > > + const GValue *value,
> > > + GParamSpec *pspec)
> > > +{
> > > + RedCharDeviceSpiceVmc *self =
> > > RED_CHAR_DEVICE_SPICEVMC(object);
> > > +
> > > + switch (property_id)
> > > + {
> > > + case PROP_CHANNEL_TYPE:
> > > + self->channel_type = g_value_get_uint(value);
> > > + break;
> > > + default:
> > > + G_OBJECT_WARN_INVALID_PROPERTY_ID(object, property_id,
> > > pspec);
> > > + }
> > > +}
> > > +
> > > +static void
> > > +red_char_device_spicevmc_constructed(GObject *object)
> > > +{
> > > + RedCharDeviceSpiceVmc *self =
> > > RED_CHAR_DEVICE_SPICEVMC(object);
> > > + RedsState *reds =
> > > red_char_device_get_server(RED_CHAR_DEVICE(self));
> > > +
> > > + self->channel = red_vmc_channel_new(reds, self->channel_type,
> > > self);
> > > + g_object_set(self, "opaque", self->channel, NULL);
> > > +}
> > > +
> > > static void
> > > red_char_device_spicevmc_class_init(RedCharDeviceSpiceVmcClass
> > > *klass)
> > > {
> > > GObjectClass *object_class = G_OBJECT_CLASS(klass);
> > > RedCharDeviceClass *char_dev_class =
> > > RED_CHAR_DEVICE_CLASS(klass);
> > >
> > > + object_class->set_property =
> > > red_char_device_spicevmc_set_property;
> > > + object_class->constructed =
> > > red_char_device_spicevmc_constructed;
> > > object_class->dispose = red_char_device_spicevmc_dispose;
> > >
> > > char_dev_class->read_one_msg_from_device =
> > > spicevmc_chardev_read_msg_from_dev;
> > > char_dev_class->send_msg_to_client =
> > > spicevmc_chardev_send_msg_to_client;
> > > char_dev_class->send_tokens_to_client =
> > > spicevmc_char_dev_send_tokens_to_client;
> > > char_dev_class->remove_client =
> > > spicevmc_char_dev_remove_client;
> > > +
> > > + g_object_class_install_property(object_class,
> > > + PROP_CHANNEL_TYPE,
> > > + g_param_spec_uint("channel-
> > > type",
> > > + "Channel
> > > type",
> > > + "Channel
> > > type for this
> > > device",
> > > + 0,
> > > G_MAXUINT, 0,
> > > + G_PARAM_WRIT
> > > ABLE |
> > > + G_PARAM_CONS
> > > TRUCT_ONLY
> > > >
> > > >
> > > +
> > > G_PARAM_STATIC_STRINGS));
> > > }
> > >
> > > static void
> > > @@ -944,13 +988,13 @@
> > > red_char_device_spicevmc_init(RedCharDeviceSpiceVmc
> > > *self)
> > > static RedCharDevice *
> > > red_char_device_spicevmc_new(SpiceCharDeviceInstance *sin,
> > > RedsState *reds,
> > > - void *opaque)
> > > + uint8_t channel_type)
> > > {
> > > return g_object_new(RED_TYPE_CHAR_DEVICE_SPICEVMC,
> > > "sin", sin,
> > > "spice-server", reds,
> > > "client-tokens-interval", 0ULL,
> > > "self-tokens", ~0ULL,
> > > - "opaque", opaque,
> > > + "channel-type", channel_type,
> > > NULL);
> > > }
> >
> > Lot of changes are related to the way "sin" are retrieved.
> > Honestly I don't like the g_object_get was, it's less type safe,
> > produce more code and it's harder to maintain, I would prefer
> > a get function.
>
>
> True, but it seems to me that the 'sin' parameter really belongs to the
> char device rather than the channel. And I don't really like having it
> duplicated in both places. Since the channel has a reference to the
> char device it can easily retrieve the 'sin' object from the char
> device. It's true that g_object_get() is not very nice, but we could
> solve that by adding a convenience function to RedCharDevice for
> accessing this property.
>
> So we could use something like:
> sin = red_char_device_get_device_instance(channel->chardev)
>
> instead of:
> g_object_get(channel->chardev, "sin", &sin, NULL);
>
>
> That would improve type-safety while still putting the property where I
> think it more properly belongs. Or would you still rather use the old
> approach?
>
> Jonathon
>
My main concern with sin and this patch is that rationale was not
mentioning it. It's true, would be better to read sin from chardev
however there may path that lead to chardev to be invalid but sin to
be still valid. Potentially the channel could outlive the device.
There are different check for this, I don't know if they are theoretical
or real but you also remove some of them and reading sin passing from
device could add other cases like this (in this case you'll have a NULL
pointer dereference).
The channel _should_ be released when red_channel_destroy is called.
However there are still some notes (if I remember in red-channel.c)
stating that a channel could take more time.
I don't have all reference counting in memory to check if passing from
the device to get the sin is leading to potential NULL dereferences.
Also it's true that device is currently never deleted but maybe that
this could potentially happen when we start adding clean up code.
At least separating the two piece of code can help reverting it.
About red_char_device_get_device_instance I'd say yes.
Frediano
More information about the Spice-devel
mailing list