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

[Frediano Ziglio]

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 at 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);