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

From: David Vrabel <david.vrabel@citrix.com>

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@citrix.com>
---
 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;
 		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);
-- 
1.7.2.5

On Tue, 2012-08-21 at 17:24 +0100, David Vrabel wrote:
> From: David Vrabel <david.vrabel@citrix.com>
> 
> 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@citrix.com>
> ---
>  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@citrix.com>
>  		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);

Ian Campbell writes ("Re: [Xen-devel] [PATCH] xenconsoled: clean-up after
all dead domains"):
> On Tue, 2012-08-21 at 17:24 +0100, David Vrabel wrote:
> > From: David Vrabel <david.vrabel@citrix.com>
Thanks, and to Ian for the review.
> Acked-by: Ian Campbell <ian.campbell@citrix.com>
Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>

Good for 4.2.

Ian.

On Thu, 2012-08-30 at 16:14 +0100, Ian Jackson wrote:
> Ian Campbell writes ("Re: [Xen-devel] [PATCH] xenconsoled: clean-up
after all dead domains"):
> > On Tue, 2012-08-21 at 17:24 +0100, David Vrabel wrote:
> > > From: David Vrabel <david.vrabel@citrix.com>
> 
> Thanks, and to Ian for the review.
> 
> > Acked-by: Ian Campbell <ian.campbell@citrix.com>
> 
> Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>
> 
> Good for 4.2.
Applied.