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

[Victor Toso]

Hi,

On Fri, Feb 22, 2019 at 10:01:00AM +0000, Frediano Ziglio wrote:
> 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.

Not particular familiar with the possibilities but I can't see
any harm on this either way.

I would add more justification in the commit log just to make it
easier when checking the commit, the goal of this change. As
justification, i mean your reply to teuf:

    https://lists.freedesktop.org/archives/spice-devel/2019-March/048660.html

Also,

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

I don't mind breaking the line here

> +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)

but I find one argument per line nicer, as it was before.

>  {
>      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,

I would also move the client to a new line.

Besides the commit log, these other comments are just my opinion,
feel free to keep as is.

Cheers,

>                                              max_send_queue_size,
>                                              num_client_tokens,
>                                              num_send_tokens);
> -- 
> 2.20.1
> 
> _______________________________________________
> Spice-devel mailing list
> Spice-devel at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/spice-devel
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <https://lists.freedesktop.org/archives/spice-devel/attachments/20190403/cf7a1f3b/attachment.sig>