Re: [Spice-devel] [PATCH spice-server] char-device: Make RedClient an opaque structure again
Hi,
On Fri, Feb 22, 2019 at 10:01:00AM +, 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
> ---
> 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
> #include
> +
> #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@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/spice-devel
signature.asc
Description: PGP signature
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel
Re: [Spice-devel] [PATCH spice-server] char-device: Make RedClient an opaque structure again
On Tue, Mar 12, 2019 at 05:58:40AM -0400, Frediano Ziglio wrote:
> ping
I guess I would reword this as "char-device: Don't use RedClient API"
RedCharDevice only use red_client_get_server() once, we can store a
Reds* in RedCharDeviceClient instead. This will make it possible to turn
the RedClient reference into a generic pointer/handle in a later commit,
which will be useful if we want to split the flow control part of
char-device in a more specialised file
(not sure the last part is accurate regarding your goal ;)
Dunno how that sounds?
Christophe
>
> >
> > 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
> > ---
> > 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
> > #include
> > +
> > #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
signature.asc
Description: PGP signature
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel
Re: [Spice-devel] [PATCH spice-server] char-device: Make RedClient an opaque structure again
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
> > > > ---
> > > > 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
> > > > #include
> > > > +
> > > > #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
Re: [Spice-devel] [PATCH spice-server] char-device: Make RedClient an opaque structure again
>
> 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
> > > ---
> > > 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
> > > #include
> > > +
> > > #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
>
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel
Re: [Spice-devel] [PATCH spice-server] char-device: Make RedClient an opaque structure again
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).
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?
Christophe
> >
> > Signed-off-by: Frediano Ziglio
> > ---
> > 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
> > #include
> > +
> > #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
signature.asc
Description: PGP signature
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel
Re: [Spice-devel] [PATCH spice-server] char-device: Make RedClient an opaque structure again
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
> ---
> 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
> #include
> +
> #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
[Spice-devel] [PATCH spice-server] char-device: Make RedClient an opaque structure again
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
---
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
#include
+
#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);
--
2.20.1
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel
