[Spice-devel] [RFC PATCH spice-server v2 06/19] stream-device: Create channel for stream device

[Frediano Ziglio]

> 
> On Wed, 2017-06-14 at 16:39 +0100, Frediano Ziglio wrote:
> > So can be used by the device to communicate with the clients.
> > 
> > Signed-off-by: Frediano Ziglio <fziglio at redhat.com>
> > ---
> >  server/stream-device.c | 20 ++++++++++++++++++++
> >  1 file changed, 20 insertions(+)
> > 
> > diff --git a/server/stream-device.c b/server/stream-device.c
> > index 47eb3ac..6c4eccb 100644
> > --- a/server/stream-device.c
> > +++ b/server/stream-device.c
> > @@ -22,6 +22,8 @@
> >  #include <spice/stream-device.h>
> >  
> >  #include "char-device.h"
> > +#include "stream-channel.h"
> > +#include "reds.h"
> >  
> >  #define TYPE_STREAM_DEVICE stream_device_get_type()
> >  
> > @@ -37,9 +39,11 @@ typedef struct StreamDeviceClass
> > StreamDeviceClass;
> >  
> >  struct StreamDevice {
> >      RedCharDevice parent;
> > +
> >      StreamDevHeader hdr;
> >      uint8_t hdr_pos;
> >      bool has_error;
> > +    StreamChannel *channel;
> >  };
> >  
> >  struct StreamDeviceClass {
> > @@ -189,7 +193,10 @@ stream_device_connect(RedsState *reds,
> > SpiceCharDeviceInstance *sin)
> >  {
> >      SpiceCharDeviceInterface *sif;
> >  
> > +    StreamChannel *channel = stream_channel_new(reds);
> > +
> >      StreamDevice *dev = stream_device_new(sin, reds);
> > +    dev->channel = channel;
> 
> Perhaps pass channel as an argument to stream_device_new() similar to
> how we do with red_char_device_spicevmc_new()?
> 

If you mean the assign inside stream_device_new is fine for me.
If you mean write some property code I don't like it.

> >  
> >      sif = spice_char_device_get_interface(sin);
> >      if (sif->state) {
> > @@ -202,6 +209,19 @@ stream_device_connect(RedsState *reds,
> > SpiceCharDeviceInstance *sin)
> >  static void
> >  stream_device_dispose(GObject *object)
> >  {
> > +    StreamDevice *device = STREAM_DEVICE(object);
> > +
> > +    if (device->channel) {
> > +        RedChannel *red_channel = RED_CHANNEL(device->channel);
> > +        RedsState *reds = red_channel_get_server(red_channel);
> > +
> > +        // prevent future connection
> > +        reds_unregister_channel(reds, red_channel);
> > +
> > +        // close all current connections and drop the reference
> > +        red_channel_destroy(red_channel);
> > +        device->channel = NULL;
> > +    }
> 
> This function is also very similar to
> red_char_device_spicevmc_dispose(). I'm not sure it's worthwhile to
> extract a common base class for these, but I'm wondering if you
> considered it?
> 

Considered, just I didn't have a good name for it.
Seems silly but unregistering and destroying the object are quite
separate. Maybe a red_channel_shutdown? A red_channel_close would
just sound like a "close all connections" but in my mind does not
suggest that channel is unregistered. Maybe red_channel_destroy
should do the unregistering job? unregistering is called before
red_channel_destroy in all cases except for the main_channel; this
would cause a warning then the main channel is destroyed (I would
personally remove the warning from the code).

> 
> >  }
> >  
> >  static void
> 

Frediano