Re: [Xen-devel] [PATCH] xenconsoled: clean-up after all dead domains

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

On Tue, 2012-08-21 at 17:24 +0100, David Vrabel wrote:
> From: David Vrabel <david.vrabel@xxxxxxxxxx>
> 
> xenconsoled expected domains that are being shutdown to end up in the
> the DYING state and would only clean-up such domains.  HVM domains
> either didn't enter the DYING state or weren't in long enough for
> xenconsoled to notice.
> 
> For every shutdown HVM domain, xenconsoled would leak memory, grow its
> list of domains and (if guest console logging was enabled) leak the
> log file descriptor.  If the file descriptors were leaked and enough
> HVM domains were shutdown, no more console connections would work as
> the evtchn device could not be opened.  Guests would then block
> waiting to send console output.
> 
> Fix this by tagging domains that exist in enum_domains().  Afterwards,
> all untagged domains are assumed to be dead and are shutdown and
> cleaned up.
> 
> Signed-off-by: David Vrabel <david.vrabel@xxxxxxxxxx>
> ---
>  tools/console/daemon/io.c |   12 +++++++++++-
>  1 files changed, 11 insertions(+), 1 deletions(-)
> 
> diff --git a/tools/console/daemon/io.c b/tools/console/daemon/io.c
> index f09d63a..592085f 100644
> --- a/tools/console/daemon/io.c
> +++ b/tools/console/daemon/io.c
> @@ -84,6 +84,7 @@ struct domain {
>       int slave_fd;
>       int log_fd;
>       bool is_dead;
> +     unsigned last_seen;
>       struct buffer buffer;
>       struct domain *next;
>       char *conspath;
> @@ -727,12 +728,16 @@ static void shutdown_domain(struct domain *d)
>       d->xce_handle = NULL;
>  }
>  
> +static unsigned enum_pass = 0;
> +
>  void enum_domains(void)
>  {
>       int domid = 1;
>       xc_dominfo_t dominfo;
>       struct domain *dom;
>  
> +     enum_pass++;
> +
>       while (xc_domain_getinfo(xc, domid, 1, &dominfo) == 1) {
>               dom = lookup_domain(dominfo.domid);
>               if (dominfo.dying) {
> @@ -740,8 +745,10 @@ void enum_domains(void)
>                               shutdown_domain(dom);
>               } else {
>                       if (dom == NULL)
> -                             create_domain(dominfo.domid);
> +                             dom = create_domain(dominfo.domid);
>               }
> +             if (dom)
> +                     dom->last_seen = enum_pass;

Dereferencing dom here safe if you took the 
dominfo.dying => shutdown_domain path above because that doesn't free
dom, it sets is_dead which is picked up by the cleanup_domain below.

Setting last_seen in this case avoids calling shutdown_domain twice,
which is good, if a little subtle.

I also had a look for threading since that would cause this to break
down but this function is only every called from the same thread as is
running handle_io, I was concerned because xs watches are involved but
it's ok.

So:
Acked-by: Ian Campbell <ian.campbell@xxxxxxxxxx>

>               domid = dominfo.domid + 1;
>       }
>  }
> @@ -1068,6 +1075,9 @@ void handle_io(void)
>                       if (d->master_fd != -1 && FD_ISSET(d->master_fd,
>                                                          &writefds))
>                               handle_tty_write(d);
> +                     
> +                     if (d->last_seen != enum_pass)
> +                             shutdown_domain(d);
>  
>                       if (d->is_dead)
>                               cleanup_domain(d);



_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel