Re: [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 
> > > ---
> > >  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 
> > >  
> > >  #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.
> 

Something like this:


diff --git a/server/stream-device.c b/server/stream-device.c
index 026f79c7..3a41bbce 100644
--- a/server/stream-device.c
+++ b/server/stream-device.c
@@ -51,7 +51,8 @@ struct StreamDeviceClass {
 };
 
 static GType stream_device_get_type(void) G_GNUC_CONST;
-static StreamDevice *stream_device_new(SpiceCharDeviceInstance *sin, RedsState 
*reds);
+static StreamDevice *stream_device_new(SpiceCharDeviceInstance *sin, RedsState 
*reds,
+   StreamChannel *channel);
 
 G_DEFINE_TYPE(StreamDevice, stream_device, RED_TYPE_CHAR_DEVICE)
 
@@ -195,7 +196,7 @@ stream_device_connect(RedsState *reds, 
SpiceCharDeviceInstance *sin)
 
 StreamChannel *channel = stream_channel_new(reds);
 
-StreamDevice *dev = stream_device_new(sin, reds);
+StreamDevice *dev = stream_device_new(sin, reds, channel);
 dev->channel = channel;
 
 sif = spice_char_device_get_interface(sin);
@@ -244,12 +245,15 @@ stream_device_init(StreamDevice *self)
 }
 
 static StreamDevice *
-stream_device_new(SpiceCharDeviceInstance *sin, RedsState *reds)
+stream_device_new(SpiceCharDeviceInstance *sin, RedsState *reds, StreamChannel 
*channel)
 {
-return g_object_new(TYPE_STREAM_DEVICE,
-"sin", sin,
-"spice-server", reds,
-"client-tokens-interval", 0ULL,
-"self-tokens", ~0ULL,
-NULL);
+StreamDevice *dev;
+dev = g_object_new(TYPE_STREAM_DEVICE,
+   "sin", sin,
+   "spice-server", reds,
+   "client-tokens-interval", 0ULL,
+   "self-tokens", ~0ULL,
+   NULL);
+dev->channel = channel;
+return dev;
 }

Frediano
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [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 
> > ---
> >  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 
> >  
> >  #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
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

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

[Jonathon Jongsma]

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 
> ---
>  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 
>  
>  #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()? 

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


>  }
>  
>  static void
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

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

[Frediano Ziglio]

So can be used by the device to communicate with the clients.

Signed-off-by: Frediano Ziglio 
---
 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 
 
 #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;
 
 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;
+}
 }
 
 static void
-- 
2.9.4

___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel