Re: [Spice-devel] [PATCH spice-server v2 5/8] Add CommonGraphicsChannelPrivate struct

[Frediano Ziglio]

> 
> On Mon, 2016-10-10 at 12:20 -0400, Frediano Ziglio wrote:
> > > 
> > > 
> > > From: Jonathon Jongsma 
> > > 
> > > Encapsulate private data for CommonGraphicsChannel and prepare for
> > > GObject conversion.
> > 
> > This object has all the fields with accessors... not a really private
> > I would say. Is not possible to implement in a different way using
> > this structure in CursorChannelPrivate and DisplayChannelPrivate ?
> 
> Possibly, but I'm not sure that I see the advantage of doing so. Also
> note that the DisplayChannelClient currently uses some of these
> variables, so we still need to access/modify this data from outside of
> the DisplayChannel implementation.
> 

Well, dcc include display-channel-private.h so it's not an issue.
I was just wondering if in GObject there is such feature of sharing
some private behavior.

> 
> > 
> > > 
> > > ---
> > >  server/common-graphics-channel.c | 34
> > > --
> > >  server/common-graphics-channel.h | 14 ++
> > >  server/cursor-channel-client.c   |  2 +-
> > >  server/cursor-channel.c  |  8 +---
> > >  server/dcc-send.c|  2 +-
> > >  server/dcc.c |  8 
> > >  server/display-channel.c |  6 +++---
> > >  server/red-worker.c  |  7 ---
> > >  8 files changed, 56 insertions(+), 25 deletions(-)
> > > 
> > > diff --git a/server/common-graphics-channel.c
> > > b/server/common-graphics-channel.c
> > > index bcf7279..6871540 100644
> > > --- a/server/common-graphics-channel.c
> > > +++ b/server/common-graphics-channel.c
> > > @@ -27,6 +27,20 @@
> > >  #include "dcc.h"
> > >  #include "main-channel-client.h"
> > >  
> > > +#define CHANNEL_RECEIVE_BUF_SIZE 1024
> > > +
> > > +struct CommonGraphicsChannelPrivate
> > > +{
> > > +QXLInstance *qxl;
> > > +uint8_t recv_buf[CHANNEL_RECEIVE_BUF_SIZE];
> > > +uint32_t id_alloc; // bitfield. TODO - use this instead of
> > > shift scheme.
> > 
> > No reason to introduced again this unused field.
> 
> Oops, must have slipped back in during the rebase.
> 
> > 
> > > @@ -123,7 +137,23 @@ CommonGraphicsChannel*
> > > common_graphics_channel_new(RedsState *server,
> > >  spice_return_val_if_fail(channel, NULL);
> > >  
> > >  common = COMMON_GRAPHICS_CHANNEL(channel);
> > > -common->qxl = qxl;
> > > +common->priv = g_new0(CommonGraphicsChannelPrivate, 1);
> > > +common->priv->qxl = qxl;
> > >  return common;
> > >  }
> > >  
> > 
> > new and no free, leak... but only till next patch so it's not a big
> > issue.
> 
> Hmm, yeah. We could do the 1-element array trick again, but that would
> require us to have the private struct definition available in the
> header. Not sure it's worth it.
> 
> 
> > 
> > > 
> > > +void
> > > common_graphics_channel_set_during_target_migrate(CommonGraphicsCha
> > > nnel
> > > *self, gboolean value)
> > > +{
> > > +self->priv->during_target_migrate = value;
> > > +}
> > > +
> > > +gboolean
> > > common_graphics_channel_get_during_target_migrate(CommonGraphicsCha
> > > nnel
> > > *self)
> > > +{
> > > +return self->priv->during_target_migrate;
> > > +}
> > > +
> > 
> > This field should really not be in this "private" structure
> 
> Can you expand on this comment? Where do you think it should be?
> 
> 
> Jonathon
> 

I was just thinking if staying in CommonGraphicsChannel instead of having
some fake private field is not better in this case.

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

Re: [Spice-devel] [PATCH spice-server v2 5/8] Add CommonGraphicsChannelPrivate struct

[Jonathon Jongsma]

On Mon, 2016-10-10 at 12:20 -0400, Frediano Ziglio wrote:
> > 
> > 
> > From: Jonathon Jongsma 
> > 
> > Encapsulate private data for CommonGraphicsChannel and prepare for
> > GObject conversion.
> 
> This object has all the fields with accessors... not a really private
> I would say. Is not possible to implement in a different way using
> this structure in CursorChannelPrivate and DisplayChannelPrivate ?

Possibly, but I'm not sure that I see the advantage of doing so. Also
note that the DisplayChannelClient currently uses some of these
variables, so we still need to access/modify this data from outside of
the DisplayChannel implementation. 


> 
> > 
> > ---
> >  server/common-graphics-channel.c | 34
> > --
> >  server/common-graphics-channel.h | 14 ++
> >  server/cursor-channel-client.c   |  2 +-
> >  server/cursor-channel.c  |  8 +---
> >  server/dcc-send.c|  2 +-
> >  server/dcc.c |  8 
> >  server/display-channel.c |  6 +++---
> >  server/red-worker.c  |  7 ---
> >  8 files changed, 56 insertions(+), 25 deletions(-)
> > 
> > diff --git a/server/common-graphics-channel.c
> > b/server/common-graphics-channel.c
> > index bcf7279..6871540 100644
> > --- a/server/common-graphics-channel.c
> > +++ b/server/common-graphics-channel.c
> > @@ -27,6 +27,20 @@
> >  #include "dcc.h"
> >  #include "main-channel-client.h"
> >  
> > +#define CHANNEL_RECEIVE_BUF_SIZE 1024
> > +
> > +struct CommonGraphicsChannelPrivate
> > +{
> > +QXLInstance *qxl;
> > +uint8_t recv_buf[CHANNEL_RECEIVE_BUF_SIZE];
> > +uint32_t id_alloc; // bitfield. TODO - use this instead of
> > shift scheme.
> 
> No reason to introduced again this unused field.

Oops, must have slipped back in during the rebase.

> 
> > @@ -123,7 +137,23 @@ CommonGraphicsChannel*
> > common_graphics_channel_new(RedsState *server,
> >  spice_return_val_if_fail(channel, NULL);
> >  
> >  common = COMMON_GRAPHICS_CHANNEL(channel);
> > -common->qxl = qxl;
> > +common->priv = g_new0(CommonGraphicsChannelPrivate, 1);
> > +common->priv->qxl = qxl;
> >  return common;
> >  }
> >  
> 
> new and no free, leak... but only till next patch so it's not a big
> issue.

Hmm, yeah. We could do the 1-element array trick again, but that would
require us to have the private struct definition available in the
header. Not sure it's worth it.


> 
> > 
> > +void
> > common_graphics_channel_set_during_target_migrate(CommonGraphicsCha
> > nnel
> > *self, gboolean value)
> > +{
> > +self->priv->during_target_migrate = value;
> > +}
> > +
> > +gboolean
> > common_graphics_channel_get_during_target_migrate(CommonGraphicsCha
> > nnel
> > *self)
> > +{
> > +return self->priv->during_target_migrate;
> > +}
> > +
> 
> This field should really not be in this "private" structure

Can you expand on this comment? Where do you think it should be?


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

Re: [Spice-devel] [PATCH spice-server v2 5/8] Add CommonGraphicsChannelPrivate struct

[Frediano Ziglio]

> 
> From: Jonathon Jongsma 
> 
> Encapsulate private data for CommonGraphicsChannel and prepare for
> GObject conversion.

This object has all the fields with accessors... not a really private
I would say. Is not possible to implement in a different way using
this structure in CursorChannelPrivate and DisplayChannelPrivate ?

> ---
>  server/common-graphics-channel.c | 34 --
>  server/common-graphics-channel.h | 14 ++
>  server/cursor-channel-client.c   |  2 +-
>  server/cursor-channel.c  |  8 +---
>  server/dcc-send.c|  2 +-
>  server/dcc.c |  8 
>  server/display-channel.c |  6 +++---
>  server/red-worker.c  |  7 ---
>  8 files changed, 56 insertions(+), 25 deletions(-)
> 
> diff --git a/server/common-graphics-channel.c
> b/server/common-graphics-channel.c
> index bcf7279..6871540 100644
> --- a/server/common-graphics-channel.c
> +++ b/server/common-graphics-channel.c
> @@ -27,6 +27,20 @@
>  #include "dcc.h"
>  #include "main-channel-client.h"
>  
> +#define CHANNEL_RECEIVE_BUF_SIZE 1024
> +
> +struct CommonGraphicsChannelPrivate
> +{
> +QXLInstance *qxl;
> +uint8_t recv_buf[CHANNEL_RECEIVE_BUF_SIZE];
> +uint32_t id_alloc; // bitfield. TODO - use this instead of shift scheme.

No reason to introduced again this unused field.

> +int during_target_migrate; /* TRUE when the client that is associated
> with the channel
> +  is during migration. Turned off when the
> vm is started.
> +  The flag is used to avoid sending messages
> that are artifacts
> +  of the transition from stopped vm to
> loaded vm (e.g., recreation
> +  of the primary surface) */
> +};
> +
>  static uint8_t *common_alloc_recv_buf(RedChannelClient *rcc, uint16_t type,
>  uint32_t size)
>  {
>  RedChannel *channel = red_channel_client_get_channel(rcc);
> @@ -41,7 +55,7 @@ static uint8_t *common_alloc_recv_buf(RedChannelClient
> *rcc, uint16_t type, uint
>  spice_critical("unexpected message size %u (max is %d)", size,
>  CHANNEL_RECEIVE_BUF_SIZE);
>  return NULL;
>  }
> -return common->recv_buf;
> +return common->priv->recv_buf;
>  }
>  
>  static void common_release_recv_buf(RedChannelClient *rcc, uint16_t type,
>  uint32_t size,
> @@ -123,7 +137,23 @@ CommonGraphicsChannel*
> common_graphics_channel_new(RedsState *server,
>  spice_return_val_if_fail(channel, NULL);
>  
>  common = COMMON_GRAPHICS_CHANNEL(channel);
> -common->qxl = qxl;
> +common->priv = g_new0(CommonGraphicsChannelPrivate, 1);
> +common->priv->qxl = qxl;
>  return common;
>  }
>  

new and no free, leak... but only till next patch so it's not a big
issue.

> +void common_graphics_channel_set_during_target_migrate(CommonGraphicsChannel
> *self, gboolean value)
> +{
> +self->priv->during_target_migrate = value;
> +}
> +
> +gboolean
> common_graphics_channel_get_during_target_migrate(CommonGraphicsChannel
> *self)
> +{
> +return self->priv->during_target_migrate;
> +}
> +

This field should really not be in this "private" structure

> +QXLInstance* common_graphics_channel_get_qxl(CommonGraphicsChannel *self)
> +{
> +return self->priv->qxl;
> +}
> +
> diff --git a/server/common-graphics-channel.h
> b/server/common-graphics-channel.h
> index 7b2aeff..c6c3f48 100644
> --- a/server/common-graphics-channel.h
> +++ b/server/common-graphics-channel.h
> @@ -25,21 +25,19 @@ int common_channel_config_socket(RedChannelClient *rcc);
>  
>  #define COMMON_CLIENT_TIMEOUT (NSEC_PER_SEC * 30)
>  
> -#define CHANNEL_RECEIVE_BUF_SIZE 1024
> +typedef struct CommonGraphicsChannelPrivate CommonGraphicsChannelPrivate;
>  typedef struct CommonGraphicsChannel {
>  RedChannel base; // Must be the first thing
>  
> -QXLInstance *qxl;
> -uint8_t recv_buf[CHANNEL_RECEIVE_BUF_SIZE];
> -int during_target_migrate; /* TRUE when the client that is associated
> with the channel
> -  is during migration. Turned off when the
> vm is started.
> -  The flag is used to avoid sending messages
> that are artifacts
> -  of the transition from stopped vm to
> loaded vm (e.g., recreation
> -  of the primary surface) */
> +CommonGraphicsChannelPrivate *priv;
>  } CommonGraphicsChannel;
>  
>  #define COMMON_GRAPHICS_CHANNEL(Channel) ((CommonGraphicsChannel*)(Channel))
>  
> +void common_graphics_channel_set_during_target_migrate(CommonGraphicsChannel
> *self, gboolean value);
> +gboolean
> common_graphics_channel_get_during_target_migrate(CommonGraphicsChannel
> *self);
> +QXLInstance* common_graphics_channel_get_qxl(CommonGraphicsChannel *self);
> +
>  enum {
>  RED_PIPE_ITEM_TYPE_VERB = 

[Spice-devel] [PATCH spice-server v2 5/8] Add CommonGraphicsChannelPrivate struct

[Frediano Ziglio]

From: Jonathon Jongsma 

Encapsulate private data for CommonGraphicsChannel and prepare for
GObject conversion.
---
 server/common-graphics-channel.c | 34 --
 server/common-graphics-channel.h | 14 ++
 server/cursor-channel-client.c   |  2 +-
 server/cursor-channel.c  |  8 +---
 server/dcc-send.c|  2 +-
 server/dcc.c |  8 
 server/display-channel.c |  6 +++---
 server/red-worker.c  |  7 ---
 8 files changed, 56 insertions(+), 25 deletions(-)

diff --git a/server/common-graphics-channel.c b/server/common-graphics-channel.c
index bcf7279..6871540 100644
--- a/server/common-graphics-channel.c
+++ b/server/common-graphics-channel.c
@@ -27,6 +27,20 @@
 #include "dcc.h"
 #include "main-channel-client.h"
 
+#define CHANNEL_RECEIVE_BUF_SIZE 1024
+
+struct CommonGraphicsChannelPrivate
+{
+QXLInstance *qxl;
+uint8_t recv_buf[CHANNEL_RECEIVE_BUF_SIZE];
+uint32_t id_alloc; // bitfield. TODO - use this instead of shift scheme.
+int during_target_migrate; /* TRUE when the client that is associated with 
the channel
+  is during migration. Turned off when the vm 
is started.
+  The flag is used to avoid sending messages 
that are artifacts
+  of the transition from stopped vm to loaded 
vm (e.g., recreation
+  of the primary surface) */
+};
+
 static uint8_t *common_alloc_recv_buf(RedChannelClient *rcc, uint16_t type, 
uint32_t size)
 {
 RedChannel *channel = red_channel_client_get_channel(rcc);
@@ -41,7 +55,7 @@ static uint8_t *common_alloc_recv_buf(RedChannelClient *rcc, 
uint16_t type, uint
 spice_critical("unexpected message size %u (max is %d)", size, 
CHANNEL_RECEIVE_BUF_SIZE);
 return NULL;
 }
-return common->recv_buf;
+return common->priv->recv_buf;
 }
 
 static void common_release_recv_buf(RedChannelClient *rcc, uint16_t type, 
uint32_t size,
@@ -123,7 +137,23 @@ CommonGraphicsChannel* 
common_graphics_channel_new(RedsState *server,
 spice_return_val_if_fail(channel, NULL);
 
 common = COMMON_GRAPHICS_CHANNEL(channel);
-common->qxl = qxl;
+common->priv = g_new0(CommonGraphicsChannelPrivate, 1);
+common->priv->qxl = qxl;
 return common;
 }
 
+void common_graphics_channel_set_during_target_migrate(CommonGraphicsChannel 
*self, gboolean value)
+{
+self->priv->during_target_migrate = value;
+}
+
+gboolean 
common_graphics_channel_get_during_target_migrate(CommonGraphicsChannel *self)
+{
+return self->priv->during_target_migrate;
+}
+
+QXLInstance* common_graphics_channel_get_qxl(CommonGraphicsChannel *self)
+{
+return self->priv->qxl;
+}
+
diff --git a/server/common-graphics-channel.h b/server/common-graphics-channel.h
index 7b2aeff..c6c3f48 100644
--- a/server/common-graphics-channel.h
+++ b/server/common-graphics-channel.h
@@ -25,21 +25,19 @@ int common_channel_config_socket(RedChannelClient *rcc);
 
 #define COMMON_CLIENT_TIMEOUT (NSEC_PER_SEC * 30)
 
-#define CHANNEL_RECEIVE_BUF_SIZE 1024
+typedef struct CommonGraphicsChannelPrivate CommonGraphicsChannelPrivate;
 typedef struct CommonGraphicsChannel {
 RedChannel base; // Must be the first thing
 
-QXLInstance *qxl;
-uint8_t recv_buf[CHANNEL_RECEIVE_BUF_SIZE];
-int during_target_migrate; /* TRUE when the client that is associated with 
the channel
-  is during migration. Turned off when the vm 
is started.
-  The flag is used to avoid sending messages 
that are artifacts
-  of the transition from stopped vm to loaded 
vm (e.g., recreation
-  of the primary surface) */
+CommonGraphicsChannelPrivate *priv;
 } CommonGraphicsChannel;
 
 #define COMMON_GRAPHICS_CHANNEL(Channel) ((CommonGraphicsChannel*)(Channel))
 
+void common_graphics_channel_set_during_target_migrate(CommonGraphicsChannel 
*self, gboolean value);
+gboolean 
common_graphics_channel_get_during_target_migrate(CommonGraphicsChannel *self);
+QXLInstance* common_graphics_channel_get_qxl(CommonGraphicsChannel *self);
+
 enum {
 RED_PIPE_ITEM_TYPE_VERB = RED_PIPE_ITEM_TYPE_CHANNEL_BASE,
 RED_PIPE_ITEM_TYPE_INVAL_ONE,
diff --git a/server/cursor-channel-client.c b/server/cursor-channel-client.c
index 90778ed..b7ab2e5 100644
--- a/server/cursor-channel-client.c
+++ b/server/cursor-channel-client.c
@@ -124,7 +124,7 @@ CursorChannelClient* 
cursor_channel_client_new(CursorChannel *cursor, RedClient
  "common-caps", common_caps_array,
  "caps", caps_array,
  NULL);
-COMMON_GRAPHICS_CHANNEL(cursor)->during_target_migrate = mig_target;
+
common_graphics_channel_set_during_target_migrate(COMMON_GRAPHICS_CHANNEL(cursor),