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
_______________________________________________
Spice-devel mailing list
Spice-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/spice-devel