[Spice-devel] [PATCH] fixup! spicevmc: Change creation of RedCharDeviceSpiceVmc

[Frediano Ziglio]

> 
> ---
> Possible fixup to Frediano's proposed patch. In general, GObject _new()
> functions should try not to do more than call g_object_new(). This is mostly
> to
> make things simpler for language bindings, but I think it's good practice to
> do
> as much initialization via the GObject construction framework as possible. So
> I
> made 'channel' a GObject property in RedCharDeviceSpicevmc, and moved some of
> the additional work up to spicevmc_device_connect().
> 
> For the time being, I've left the duplicated 'sin' parameter in the
> RedVmcChannel. But if we removed that from the channel (and simply retrieved
> it
> as-needed from the char device as discussed in another patch), it would
> simplify things further, since spicevmc_device_connect() would not really
> have
> to do any additional work besides creating the objects.
> 
>  server/spicevmc.c | 81
>  ++++++++++++++++++++++++++++++++++++++++---------------
>  1 file changed, 59 insertions(+), 22 deletions(-)
> 
> diff --git a/server/spicevmc.c b/server/spicevmc.c
> index cf743de..084f09a 100644
> --- a/server/spicevmc.c
> +++ b/server/spicevmc.c
> @@ -89,7 +89,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,
> -                                                   uint8_t channel_type);
> +                                                   RedVmcChannel *channel);
>  
>  G_DEFINE_TYPE(RedCharDeviceSpiceVmc, red_char_device_spicevmc,
>  RED_TYPE_CHAR_DEVICE)
>  
> @@ -806,7 +806,20 @@ RedCharDevice *spicevmc_device_connect(RedsState *reds,
>                                         SpiceCharDeviceInstance *sin,
>                                         uint8_t channel_type)
>  {
> -    return red_char_device_spicevmc_new(sin, reds, channel_type);
> +    RedCharDevice *dev;
> +    RedVmcChannel *channel = red_vmc_channel_new(reds, channel_type);
> +    if (!channel) {
> +        return NULL;
> +    }
> +
> +    /* char device takes ownership of channel */
> +    dev = red_char_device_spicevmc_new(sin, reds, channel);
> +
> +    channel->chardev_sin = sin;
> +    g_object_unref(channel);
> +
> +    return dev;
>  }
>  
>  /* Must be called from RedClient handling thread. */
> @@ -861,18 +874,54 @@ red_char_device_spicevmc_dispose(GObject *object)
>      }
>  }
>  
> +enum {
> +    PROP0,
> +    PROP_CHANNEL
> +};
> +
> +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:
> +            spice_assert(self->channel == NULL);
> +            self->channel = g_value_dup_object(value);
> +            self->channel->chardev = RED_CHAR_DEVICE(self);
> +
> +            break;
> +        default:
> +            G_OBJECT_WARN_INVALID_PROPERTY_ID(object, property_id, pspec);
> +    }
> +}
> +
>  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->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,
> +                                    g_param_spec_object("channel",
> +                                                         "Channel",
> +                                                         "Channel associated
> with this device",
> +
> RED_TYPE_VMC_CHANNEL,
> +
> G_PARAM_STATIC_STRINGS
> |
> +                                                         G_PARAM_WRITABLE |
> +
> G_PARAM_CONSTRUCT));
>  }
>  
>  static void
> @@ -883,25 +932,13 @@ red_char_device_spicevmc_init(RedCharDeviceSpiceVmc
> *self)
>  static RedCharDevice *
>  red_char_device_spicevmc_new(SpiceCharDeviceInstance *sin,
>                               RedsState *reds,
> -                             uint8_t channel_type)
> +                             RedVmcChannel *channel)
>  {
> -    RedCharDeviceSpiceVmc *dev;
> -    RedVmcChannel *channel = red_vmc_channel_new(reds, channel_type);
> -    if (!channel) {
> -        return NULL;
> -    }
> -
> -    dev = g_object_new(RED_TYPE_CHAR_DEVICE_SPICEVMC,
> -                       "sin", sin,
> -                       "spice-server", reds,
> -                       "client-tokens-interval", 0ULL,
> -                       "self-tokens", ~0ULL,
> -                       "opaque", channel,
> -                       NULL);
> -
> -    channel->chardev = RED_CHAR_DEVICE(dev);
> -    channel->chardev_sin = sin;
> -    dev->channel = channel;
> -
> -    return RED_CHAR_DEVICE(dev);
> +    return g_object_new(RED_TYPE_CHAR_DEVICE_SPICEVMC,
> +                        "sin", sin,
> +                        "spice-server", reds,
> +                        "client-tokens-interval", 0ULL,
> +                        "self-tokens", ~0ULL,
> +                        "opaque", channel,
> +                        NULL);
>  }

Not adding "channel" here cause a crash. Don't know why.
Adding it fixes the problem (opaque and channel are both needed).
Actually was a bit weird... not having "channel" in the list
crashed in red_char_device_spicevmc_set_property, specifically

   self->channel->chardev = RED_CHAR_DEVICE(self);

line. But why this property is set if not passed? Perhaps
objects not passed are tried to set to NULL? Maybe.

By the way, this is a reason I don't like much too dynamic code
in a static language like C. We (just question of time before it
happens to me) are keeping sending buggy patches hard to review
and test.
This from my point of view is less maintainable code.

Frediano