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

[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 <jwhite at codeweavers.com>
> > > ---
> > >  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) {
> 
>