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

[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 <alevy at redhat.com>
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