The subject is a bit confusing to me.
Looks like is nor a "what" not a "why", just a statement.
>
> From: Jonathon Jongsma <jjongsma@xxxxxxxxxx>
>
> 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_READWRITE |
> G_PARAM_CONSTRUCT_ONLY
> |
> G_PARAM_STATIC_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_WRITABLE |
> + G_PARAM_CONSTRUCT_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.
I'll post a different proposal not using properties.
Frediano
_______________________________________________
Spice-devel mailing list
Spice-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/spice-devel