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@xxxxxxxxxxxxxxx>
> > > ---
> > > 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@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/spice-devel