[Spice-devel] [PATCH spice-server v2 1/2] spicevmc: Channel is owned by device
Jonathon Jongsma
jjongsma at redhat.com
Fri Nov 4 14:59:28 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
More information about the Spice-devel
mailing list