Re: [Spice-devel] [PATCH] Delay the exit call for the exit on disconnect function.

2016-11-03 Thread Jeremy White 
>> On 11/03/2016 04:58 AM, Pavel Grunt wrote:
>>> moving it down can cause server to not exit in case client==NULL
>>
>> I spent some time and could not persuade myself of a case where this
>> function would be called with client == NULL.  Am I missing
>> something?
> 
> Same here, you are right, but things are changing a lot recently...
> And the check is there
> 
> There are more possibilities - one of these is to make sure it will
> not return earlier, like:
> ..
> if (!reds->config->exit_on_disconnect &&
> (!client || client->disconnecting))

Hmm.  I'm concerned that obfuscates the code a bit and muddles the
question of whether or not client can be null (for example, it implies
that client cannot be null when exit_on_disconnect is set, but it
otherwise can be).

The !client check was introduced by this commit:

commit 1078dc04edc406950e5f6d91bae456411eaa4a47
Author: Alon Levy 
Date:   Tue Aug 23 14:13:16 2011 +0300

server/reds: reds_client_disconnect: remove wrong check for
reds_main_channel_connected

The "channel->disconnecting" parameter already protects against
recursion.

Removed fixed TODOs.

I don't see any evidence that the '!client' check was really required;
it seems like something Alon may have injected reflexively.  It was not
checked prior to that patch.

So I'm inclined to switch it to just:

  if (client->disconnecting)

I think that communicates our understanding to future developers more
clearly, and it parallels similar checks in other places in the code.

Thoughts?

> ..
> 
> and move the if (reds->config->exit_on_disconnect) { exit()}
> like you did (or more to the end of the function)
> 
> 
>>
>>>
>>> OT: if we consider multiple client connections
>>> (SPICE_DEBUG_ALLOW_MC=1) than it is still leaking
>>
>> Yah.  Arguably, the exit on disconnect option should be mutually
>> exclusive of the ability to allow multiple clients.  That's not
>> enforced, currently, although this behavior does enforce it at first
>> client disconnect :-/.
> 
> maybe it can exit when all the clients disconnects - if the concern is
>  to clean properly all the clients? 
> 
> (but in the end everything is cleaned on exit, no ?)

Well, my specific concern is getting the event notification so that I
can run specialized code on disconnect; it's not so much about internal
Spice cleanup.

(For full edification, I've got code in x11spice that flips the desktop
background red when someone connects, and then relies on this disconnect
notice to put the desktop back to normal).

Cheers,

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

Re: [Spice-devel] [PATCH] Delay the exit call for the exit on disconnect function.

2016-11-03 Thread Pavel Grunt 
Hi Jeremy,

On Thu, 2016-11-03 at 10:33 -0500, Jeremy White wrote:
> Hi Pavel,
> 
> Thanks for the review.
> 
> On 11/03/2016 04:58 AM, Pavel Grunt wrote:
> > moving it down can cause server to not exit in case client==NULL
> 
> I spent some time and could not persuade myself of a case where this
> function would be called with client == NULL.  Am I missing
> something?

Same here, you are right, but things are changing a lot recently...
And the check is there

There are more possibilities - one of these is to make sure it will
not return earlier, like:
..
if (!reds->config->exit_on_disconnect &&
(!client || client->disconnecting))
..

and move the if (reds->config->exit_on_disconnect) { exit()}
like you did (or more to the end of the function)


> 
> > 
> > OT: if we consider multiple client connections
> > (SPICE_DEBUG_ALLOW_MC=1) than it is still leaking
> 
> Yah.  Arguably, the exit on disconnect option should be mutually
> exclusive of the ability to allow multiple clients.  That's not
> enforced, currently, although this behavior does enforce it at first
> client disconnect :-/.

maybe it can exit when all the clients disconnects - if the concern is
 to clean properly all the clients? 

(but in the end everything is cleaned on exit, no ?)

Thanks,
Pavel

> 
> Cheers,
> 
> Jeremy
> 
> > 
> > Pavel
> > 
> > On Wed, 2016-11-02 at 10:42 -0500, Jeremy White wrote:
> > > This will allow client cleanup to happen.
> > > 
> > > Signed-off-by: Jeremy White 
> > > ---
> > >  server/reds.c | 12 ++--
> > >  1 file changed, 6 insertions(+), 6 deletions(-)
> > > 
> > > diff --git a/server/reds.c b/server/reds.c
> > > index c235421..e380dd1 100644
> > > --- a/server/reds.c
> > > +++ b/server/reds.c
> > > @@ -559,12 +559,6 @@ void reds_client_disconnect(RedsState
> > > *reds,
> > > RedClient *client)
> > >  {
> > >  RedsMigTargetClient *mig_client;
> > >  
> > > -if (reds->config->exit_on_disconnect)
> > > -{
> > > -spice_info("Exiting server because of client
> > > disconnect.\n");
> > > -exit(0);
> > > -}
> > > -
> > >  if (!client || client->disconnecting) {
> > >  spice_debug("client %p already during disconnection",
> > > client);
> > >  return;
> > > @@ -599,6 +593,12 @@ void reds_client_disconnect(RedsState
> > > *reds,
> > > RedClient *client)
> > >  reds->num_clients--;
> > >  red_client_destroy(client);
> > >  
> > > +if (reds->config->exit_on_disconnect)
> > > +{
> > > +spice_info("Exiting server because of client
> > > disconnect.\n");
> > > +exit(0);
> > > +}
> > > +
> > > // TODO: we need to handle agent properly for all
> > > clients
> > > (e.g., cut and paste, how? Maybe throw away messages
> > > // if we are in the middle of one from another client)
> > >  if (reds->num_clients == 0) {
> 
> 
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [PATCH] Delay the exit call for the exit on disconnect function.

2016-11-03 Thread Jeremy White 
Hi Pavel,

Thanks for the review.

On 11/03/2016 04:58 AM, Pavel Grunt wrote:
> moving it down can cause server to not exit in case client==NULL

I spent some time and could not persuade myself of a case where this
function would be called with client == NULL.  Am I missing something?

> 
> OT: if we consider multiple client connections
> (SPICE_DEBUG_ALLOW_MC=1) than it is still leaking

Yah.  Arguably, the exit on disconnect option should be mutually
exclusive of the ability to allow multiple clients.  That's not
enforced, currently, although this behavior does enforce it at first
client disconnect :-/.

Cheers,

Jeremy

> 
> Pavel
> 
> On Wed, 2016-11-02 at 10:42 -0500, Jeremy White wrote:
>> This will allow client cleanup to happen.
>>
>> Signed-off-by: Jeremy White 
>> ---
>>  server/reds.c | 12 ++--
>>  1 file changed, 6 insertions(+), 6 deletions(-)
>>
>> diff --git a/server/reds.c b/server/reds.c
>> index c235421..e380dd1 100644
>> --- a/server/reds.c
>> +++ b/server/reds.c
>> @@ -559,12 +559,6 @@ void reds_client_disconnect(RedsState *reds,
>> RedClient *client)
>>  {
>>  RedsMigTargetClient *mig_client;
>>  
>> -if (reds->config->exit_on_disconnect)
>> -{
>> -spice_info("Exiting server because of client
>> disconnect.\n");
>> -exit(0);
>> -}
>> -
>>  if (!client || client->disconnecting) {
>>  spice_debug("client %p already during disconnection",
>> client);
>>  return;
>> @@ -599,6 +593,12 @@ void reds_client_disconnect(RedsState *reds,
>> RedClient *client)
>>  reds->num_clients--;
>>  red_client_destroy(client);
>>  
>> +if (reds->config->exit_on_disconnect)
>> +{
>> +spice_info("Exiting server because of client
>> disconnect.\n");
>> +exit(0);
>> +}
>> +
>> // TODO: we need to handle agent properly for all clients
>> (e.g., cut and paste, how? Maybe throw away messages
>> // if we are in the middle of one from another client)
>>  if (reds->num_clients == 0) {

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

Re: [Spice-devel] [PATCH] Delay the exit call for the exit on disconnect function.

2016-11-03 Thread Pavel Grunt 
moving it down can cause server to not exit in case client==NULL

OT: if we consider multiple client connections
(SPICE_DEBUG_ALLOW_MC=1) than it is still leaking

Pavel

On Wed, 2016-11-02 at 10:42 -0500, Jeremy White wrote:
> This will allow client cleanup to happen.
> 
> Signed-off-by: Jeremy White 
> ---
>  server/reds.c | 12 ++--
>  1 file changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/server/reds.c b/server/reds.c
> index c235421..e380dd1 100644
> --- a/server/reds.c
> +++ b/server/reds.c
> @@ -559,12 +559,6 @@ void reds_client_disconnect(RedsState *reds,
> RedClient *client)
>  {
>  RedsMigTargetClient *mig_client;
>  
> -if (reds->config->exit_on_disconnect)
> -{
> -spice_info("Exiting server because of client
> disconnect.\n");
> -exit(0);
> -}
> -
>  if (!client || client->disconnecting) {
>  spice_debug("client %p already during disconnection",
> client);
>  return;
> @@ -599,6 +593,12 @@ void reds_client_disconnect(RedsState *reds,
> RedClient *client)
>  reds->num_clients--;
>  red_client_destroy(client);
>  
> +if (reds->config->exit_on_disconnect)
> +{
> +spice_info("Exiting server because of client
> disconnect.\n");
> +exit(0);
> +}
> +
> // TODO: we need to handle agent properly for all clients
> (e.g., cut and paste, how? Maybe throw away messages
> // if we are in the middle of one from another client)
>  if (reds->num_clients == 0) {
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

[Spice-devel] [PATCH] Delay the exit call for the exit on disconnect function.

2016-11-02 Thread Jeremy White 
This will allow client cleanup to happen.

Signed-off-by: Jeremy White 
---
 server/reds.c | 12 ++--
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/server/reds.c b/server/reds.c
index c235421..e380dd1 100644
--- a/server/reds.c
+++ b/server/reds.c
@@ -559,12 +559,6 @@ void reds_client_disconnect(RedsState *reds, RedClient 
*client)
 {
 RedsMigTargetClient *mig_client;
 
-if (reds->config->exit_on_disconnect)
-{
-spice_info("Exiting server because of client disconnect.\n");
-exit(0);
-}
-
 if (!client || client->disconnecting) {
 spice_debug("client %p already during disconnection", client);
 return;
@@ -599,6 +593,12 @@ void reds_client_disconnect(RedsState *reds, RedClient 
*client)
 reds->num_clients--;
 red_client_destroy(client);
 
+if (reds->config->exit_on_disconnect)
+{
+spice_info("Exiting server because of client disconnect.\n");
+exit(0);
+}
+
// TODO: we need to handle agent properly for all clients (e.g., cut 
and paste, how? Maybe throw away messages
// if we are in the middle of one from another client)
 if (reds->num_clients == 0) {
-- 
2.1.4

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