[spice-server] char-device: Make RedClient an opaque structure again

Submitted by Frediano Ziglio on Feb. 22, 2019, 10:01 a.m.

Details

Message ID 20190222100100.703-1-fziglio@redhat.com
State New
Headers show
Series "char-device: Make RedClient an opaque structure again" ( rev: 1 ) in Spice

Not browsing as part of any series.

Commit Message

Frediano Ziglio Feb. 22, 2019, 10:01 a.m.
RedClient was an opaque structure for RedCharDevice.
It started to be used when RedsState started to contain all
the global state.
Make it opaque again.

Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
---
 server/char-device.c | 16 +++++++---------
 1 file changed, 7 insertions(+), 9 deletions(-)

Patch hide | download patch | download mbox

diff --git a/server/char-device.c b/server/char-device.c
index 040b91147..465c1a125 100644
--- a/server/char-device.c
+++ b/server/char-device.c
@@ -22,8 +22,8 @@ 
 
 #include <config.h>
 #include <inttypes.h>
+
 #include "char-device.h"
-#include "red-client.h"
 #include "reds.h"
 #include "glib-compat.h"
 
@@ -703,11 +703,10 @@  void red_char_device_destroy(RedCharDevice *char_dev)
     g_object_unref(char_dev);
 }
 
-static RedCharDeviceClient *red_char_device_client_new(RedClient *client,
-                                                       int do_flow_control,
-                                                       uint32_t max_send_queue_size,
-                                                       uint32_t num_client_tokens,
-                                                       uint32_t num_send_tokens)
+static RedCharDeviceClient *
+red_char_device_client_new(RedsState *reds, RedClient *client,
+                           int do_flow_control, uint32_t max_send_queue_size,
+                           uint32_t num_client_tokens, uint32_t num_send_tokens)
 {
     RedCharDeviceClient *dev_client;
 
@@ -717,8 +716,6 @@  static RedCharDeviceClient *red_char_device_client_new(RedClient *client,
     dev_client->max_send_queue_size = max_send_queue_size;
     dev_client->do_flow_control = do_flow_control;
     if (do_flow_control) {
-        RedsState *reds = red_client_get_server(client);
-
         dev_client->wait_for_tokens_timer =
             reds_core_timer_add(reds, device_client_wait_for_tokens_timeout,
                                 dev_client);
@@ -757,7 +754,8 @@  bool red_char_device_client_add(RedCharDevice *dev,
     dev->priv->wait_for_migrate_data = wait_for_migrate_data;
 
     spice_debug("char device %p, client %p", dev, client);
-    dev_client = red_char_device_client_new(client, do_flow_control,
+    dev_client = red_char_device_client_new(dev->priv->reds, client,
+                                            do_flow_control,
                                             max_send_queue_size,
                                             num_client_tokens,
                                             num_send_tokens);

Comments

ping

> 
> RedClient was an opaque structure for RedCharDevice.
> It started to be used when RedsState started to contain all
> the global state.
> Make it opaque again.
> 
> Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
> ---
>  server/char-device.c | 16 +++++++---------
>  1 file changed, 7 insertions(+), 9 deletions(-)
> 
> diff --git a/server/char-device.c b/server/char-device.c
> index 040b91147..465c1a125 100644
> --- a/server/char-device.c
> +++ b/server/char-device.c
> @@ -22,8 +22,8 @@
>  
>  #include <config.h>
>  #include <inttypes.h>
> +
>  #include "char-device.h"
> -#include "red-client.h"
>  #include "reds.h"
>  #include "glib-compat.h"
>  
> @@ -703,11 +703,10 @@ void red_char_device_destroy(RedCharDevice *char_dev)
>      g_object_unref(char_dev);
>  }
>  
> -static RedCharDeviceClient *red_char_device_client_new(RedClient *client,
> -                                                       int do_flow_control,
> -                                                       uint32_t
> max_send_queue_size,
> -                                                       uint32_t
> num_client_tokens,
> -                                                       uint32_t
> num_send_tokens)
> +static RedCharDeviceClient *
> +red_char_device_client_new(RedsState *reds, RedClient *client,
> +                           int do_flow_control, uint32_t
> max_send_queue_size,
> +                           uint32_t num_client_tokens, uint32_t
> num_send_tokens)
>  {
>      RedCharDeviceClient *dev_client;
>  
> @@ -717,8 +716,6 @@ static RedCharDeviceClient
> *red_char_device_client_new(RedClient *client,
>      dev_client->max_send_queue_size = max_send_queue_size;
>      dev_client->do_flow_control = do_flow_control;
>      if (do_flow_control) {
> -        RedsState *reds = red_client_get_server(client);
> -
>          dev_client->wait_for_tokens_timer =
>              reds_core_timer_add(reds, device_client_wait_for_tokens_timeout,
>                                  dev_client);
> @@ -757,7 +754,8 @@ bool red_char_device_client_add(RedCharDevice *dev,
>      dev->priv->wait_for_migrate_data = wait_for_migrate_data;
>  
>      spice_debug("char device %p, client %p", dev, client);
> -    dev_client = red_char_device_client_new(client, do_flow_control,
> +    dev_client = red_char_device_client_new(dev->priv->reds, client,
> +                                            do_flow_control,
>                                              max_send_queue_size,
>                                              num_client_tokens,
>                                              num_send_tokens);

> 
> On Tue, Mar 12, 2019 at 05:58:40AM -0400, Frediano Ziglio wrote:
> > ping
> > 
> > > 
> > > RedClient was an opaque structure for RedCharDevice.
> > > It started to be used when RedsState started to contain all
> > > the global state.
> > > Make it opaque again.
> 
> It seems we don't put the same meaning to 'opaque'. For me, RedClient is
> already an opaque structure from a char-device.c perspective as it's not
> dereferencing RedClient * (ie it does not know/care which fields are
> defined in struct RedClient).
> 

Maybe "generic" ? Anything ?
The idea was to have a "RedCharDeviceClientOpaque" pointer with
RedCharDeviceClientOpaque as a typename so you could have something
like

#ifndef RedCharDeviceClientOpaque
typedef struct RedCharDeviceClientOpaque RedCharDeviceClientOpaque;
#define RedCharDeviceClientOpaque RedCharDeviceClientOpaque
#endif

in char-device.h

> This commit goes further than that as it removes any calls to
> red_client_* API. char-device.c still cares that it is handling a
> pointer to a RedClient * (as opposed to a gpointer client_handle which
> could be anything). Do you have some longer term goal with respect to
> RedClient and RedCharDevice, or is this just an isolated patch?
> 

I have a follow up that uses RedChannelClient as RedCharDeviceClientOpaque
instead.
But I still not decided the future plan but this way I can change to
RedChannel without touching RedCharDevice. There are parts of the code
handling multi-channel while other parts are broken in this respect.
Personally I think multi-channel does not make sense for this object.
Also token handling is broken (unlimited queue) and only used for the
agent.
I started splitting the agent which seemed to make more sense.
It is getting into shape and I think it would help defining what to do
with RedCharDevice. But time is scarce and progress is slow.

> Christophe
> 
> 
> > > 
> > > Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
> > > ---
> > >  server/char-device.c | 16 +++++++---------
> > >  1 file changed, 7 insertions(+), 9 deletions(-)
> > > 
> > > diff --git a/server/char-device.c b/server/char-device.c
> > > index 040b91147..465c1a125 100644
> > > --- a/server/char-device.c
> > > +++ b/server/char-device.c
> > > @@ -22,8 +22,8 @@
> > >  
> > >  #include <config.h>
> > >  #include <inttypes.h>
> > > +
> > >  #include "char-device.h"
> > > -#include "red-client.h"
> > >  #include "reds.h"
> > >  #include "glib-compat.h"
> > >  
> > > @@ -703,11 +703,10 @@ void red_char_device_destroy(RedCharDevice
> > > *char_dev)
> > >      g_object_unref(char_dev);
> > >  }
> > >  
> > > -static RedCharDeviceClient *red_char_device_client_new(RedClient
> > > *client,
> > > -                                                       int
> > > do_flow_control,
> > > -                                                       uint32_t
> > > max_send_queue_size,
> > > -                                                       uint32_t
> > > num_client_tokens,
> > > -                                                       uint32_t
> > > num_send_tokens)
> > > +static RedCharDeviceClient *
> > > +red_char_device_client_new(RedsState *reds, RedClient *client,
> > > +                           int do_flow_control, uint32_t
> > > max_send_queue_size,
> > > +                           uint32_t num_client_tokens, uint32_t
> > > num_send_tokens)
> > >  {
> > >      RedCharDeviceClient *dev_client;
> > >  
> > > @@ -717,8 +716,6 @@ static RedCharDeviceClient
> > > *red_char_device_client_new(RedClient *client,
> > >      dev_client->max_send_queue_size = max_send_queue_size;
> > >      dev_client->do_flow_control = do_flow_control;
> > >      if (do_flow_control) {
> > > -        RedsState *reds = red_client_get_server(client);
> > > -
> > >          dev_client->wait_for_tokens_timer =
> > >              reds_core_timer_add(reds,
> > >              device_client_wait_for_tokens_timeout,
> > >                                  dev_client);
> > > @@ -757,7 +754,8 @@ bool red_char_device_client_add(RedCharDevice *dev,
> > >      dev->priv->wait_for_migrate_data = wait_for_migrate_data;
> > >  
> > >      spice_debug("char device %p, client %p", dev, client);
> > > -    dev_client = red_char_device_client_new(client, do_flow_control,
> > > +    dev_client = red_char_device_client_new(dev->priv->reds, client,
> > > +                                            do_flow_control,
> > >                                              max_send_queue_size,
> > >                                              num_client_tokens,
> > >                                              num_send_tokens);
> > _______________________________________________
> > Spice-devel mailing list
> > Spice-devel@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/spice-devel
>
ping

> > 
> > On Tue, Mar 12, 2019 at 05:58:40AM -0400, Frediano Ziglio wrote:
> > > ping
> > > 
> > > > 
> > > > RedClient was an opaque structure for RedCharDevice.
> > > > It started to be used when RedsState started to contain all
> > > > the global state.
> > > > Make it opaque again.
> > 
> > It seems we don't put the same meaning to 'opaque'. For me, RedClient is
> > already an opaque structure from a char-device.c perspective as it's not
> > dereferencing RedClient * (ie it does not know/care which fields are
> > defined in struct RedClient).
> > 
> 
> Maybe "generic" ? Anything ?
> The idea was to have a "RedCharDeviceClientOpaque" pointer with
> RedCharDeviceClientOpaque as a typename so you could have something
> like
> 
> #ifndef RedCharDeviceClientOpaque
> typedef struct RedCharDeviceClientOpaque RedCharDeviceClientOpaque;
> #define RedCharDeviceClientOpaque RedCharDeviceClientOpaque
> #endif
> 
> in char-device.h
> 
> > This commit goes further than that as it removes any calls to
> > red_client_* API. char-device.c still cares that it is handling a
> > pointer to a RedClient * (as opposed to a gpointer client_handle which
> > could be anything). Do you have some longer term goal with respect to
> > RedClient and RedCharDevice, or is this just an isolated patch?
> > 
> 
> I have a follow up that uses RedChannelClient as RedCharDeviceClientOpaque
> instead.
> But I still not decided the future plan but this way I can change to
> RedChannel without touching RedCharDevice. There are parts of the code
> handling multi-channel while other parts are broken in this respect.
> Personally I think multi-channel does not make sense for this object.
> Also token handling is broken (unlimited queue) and only used for the
> agent.
> I started splitting the agent which seemed to make more sense.
> It is getting into shape and I think it would help defining what to do
> with RedCharDevice. But time is scarce and progress is slow.
> 
> > Christophe
> > 
> > 
> > > > 
> > > > Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
> > > > ---
> > > >  server/char-device.c | 16 +++++++---------
> > > >  1 file changed, 7 insertions(+), 9 deletions(-)
> > > > 
> > > > diff --git a/server/char-device.c b/server/char-device.c
> > > > index 040b91147..465c1a125 100644
> > > > --- a/server/char-device.c
> > > > +++ b/server/char-device.c
> > > > @@ -22,8 +22,8 @@
> > > >  
> > > >  #include <config.h>
> > > >  #include <inttypes.h>
> > > > +
> > > >  #include "char-device.h"
> > > > -#include "red-client.h"
> > > >  #include "reds.h"
> > > >  #include "glib-compat.h"
> > > >  
> > > > @@ -703,11 +703,10 @@ void red_char_device_destroy(RedCharDevice
> > > > *char_dev)
> > > >      g_object_unref(char_dev);
> > > >  }
> > > >  
> > > > -static RedCharDeviceClient *red_char_device_client_new(RedClient
> > > > *client,
> > > > -                                                       int
> > > > do_flow_control,
> > > > -                                                       uint32_t
> > > > max_send_queue_size,
> > > > -                                                       uint32_t
> > > > num_client_tokens,
> > > > -                                                       uint32_t
> > > > num_send_tokens)
> > > > +static RedCharDeviceClient *
> > > > +red_char_device_client_new(RedsState *reds, RedClient *client,
> > > > +                           int do_flow_control, uint32_t
> > > > max_send_queue_size,
> > > > +                           uint32_t num_client_tokens, uint32_t
> > > > num_send_tokens)
> > > >  {
> > > >      RedCharDeviceClient *dev_client;
> > > >  
> > > > @@ -717,8 +716,6 @@ static RedCharDeviceClient
> > > > *red_char_device_client_new(RedClient *client,
> > > >      dev_client->max_send_queue_size = max_send_queue_size;
> > > >      dev_client->do_flow_control = do_flow_control;
> > > >      if (do_flow_control) {
> > > > -        RedsState *reds = red_client_get_server(client);
> > > > -
> > > >          dev_client->wait_for_tokens_timer =
> > > >              reds_core_timer_add(reds,
> > > >              device_client_wait_for_tokens_timeout,
> > > >                                  dev_client);
> > > > @@ -757,7 +754,8 @@ bool red_char_device_client_add(RedCharDevice *dev,
> > > >      dev->priv->wait_for_migrate_data = wait_for_migrate_data;
> > > >  
> > > >      spice_debug("char device %p, client %p", dev, client);
> > > > -    dev_client = red_char_device_client_new(client, do_flow_control,
> > > > +    dev_client = red_char_device_client_new(dev->priv->reds, client,
> > > > +                                            do_flow_control,
> > > >                                              max_send_queue_size,
> > > >                                              num_client_tokens,
> > > >                                              num_send_tokens);