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

[Jonathon Jongsma]

On Fri, 2016-11-04 at 13:34 -0400, Frediano Ziglio wrote:
> > 
> > 
> > ---
> > 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_W
> > RITABLE |
> > +
> > 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.

That's what I get for trying to get the patch sent off quickly before
lunch break. Sorry.

Yeah, construct properties are always set, I believe. They just use
default values (NULL in this case) if they're not explicitly specified.
https://blogs.gnome.org/desrt/2012/02/26/a-gentle-introduction-to-gobje
ct-construction/

> 
> 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.

Well, you have a point. But that is the why these 'dynamic' calls are
hidden inside of a typesafe 'constructor' function (_new()). I think
that's a reasonable compromise.

Jonathon