[PATCH 1/2] xenstore: xenbus cannot be opened read-only

[PATCH 1/2] xenstore: xenbus cannot be opened read-only

[Daniel De Graaf]

In order to read keys from xenstore, the xenstore libraries need to
write the request to the xenbus socket. This means that the socket
cannot be opened read-only.

Signed-off-by: Daniel De Graaf <dgdegra@tycho.nsa.gov>
---
 tools/xenstore/xs.c |   18 ++++++++----------
 1 files changed, 8 insertions(+), 10 deletions(-)

diff --git a/tools/xenstore/xs.c b/tools/xenstore/xs.c
index c72ea83..df270f7 100644
--- a/tools/xenstore/xs.c
+++ b/tools/xenstore/xs.c
@@ -182,15 +182,13 @@ error:
 	return -1;
 }
 
-static int get_dev(const char *connect_to, unsigned long flags)
+static int get_dev(const char *connect_to)
 {
-	if (flags & XS_OPEN_READONLY)
-		return open(connect_to, O_RDONLY);
-	else
-		return open(connect_to, O_RDWR);
+	/* We cannot open read-only because requests are writes */
+	return open(connect_to, O_RDWR);
 }
 
-static struct xs_handle *get_handle(const char *connect_to, unsigned long flags)
+static struct xs_handle *get_handle(const char *connect_to)
 {
 	struct stat buf;
 	struct xs_handle *h = NULL;
@@ -202,7 +200,7 @@ static struct xs_handle *get_handle(const char *connect_to, unsigned long flags)
 	if (S_ISSOCK(buf.st_mode))
 		fd = get_socket(connect_to);
 	else
-		fd = get_dev(connect_to, flags);
+		fd = get_dev(connect_to);
 
 	if (fd == -1)
 		return NULL;
@@ -258,12 +256,12 @@ struct xs_handle *xs_open(unsigned long flags)
 	struct xs_handle *xsh = NULL;
 
 	if (flags & XS_OPEN_READONLY)
-		xsh = get_handle(xs_daemon_socket_ro(), flags);
+		xsh = get_handle(xs_daemon_socket_ro());
 	else
-		xsh = get_handle(xs_daemon_socket(), flags);
+		xsh = get_handle(xs_daemon_socket());
 
 	if (!xsh && !(flags & XS_OPEN_SOCKETONLY))
-		xsh = get_handle(xs_domain_dev(), flags);
+		xsh = get_handle(xs_domain_dev());
 
 	return xsh;
 }
-- 
1.7.6.4

[PATCH 2/2] xenstat: Use local domain names

[Daniel De Graaf]

The domain name stored in /local/domain/$domid/name is simpler to
access and is the only domain name modified by "xl rename". Use this
domain name in libxenstat's reporting.

Signed-off-by: Daniel De Graaf <dgdegra@tycho.nsa.gov>
---
 tools/xenstat/libxenstat/src/xenstat.c |   12 ++----------
 1 files changed, 2 insertions(+), 10 deletions(-)

diff --git a/tools/xenstat/libxenstat/src/xenstat.c b/tools/xenstat/libxenstat/src/xenstat.c
index 2791cc1..104655d 100644
--- a/tools/xenstat/libxenstat/src/xenstat.c
+++ b/tools/xenstat/libxenstat/src/xenstat.c
@@ -739,17 +739,9 @@ unsigned long long xenstat_tmem_succ_pers_gets(xenstat_tmem *tmem)
 
 static char *xenstat_get_domain_name(xenstat_handle *handle, unsigned int domain_id)
 {
-	char path[80], *vmpath;
+	char path[80];
 
-	snprintf(path, sizeof(path),"/local/domain/%i/vm", domain_id);
-
-	vmpath = xs_read(handle->xshandle, XBT_NULL, path, NULL);
-
-	if (vmpath == NULL)
-		return NULL;
-
-	snprintf(path, sizeof(path),"%s/name", vmpath);
-	free(vmpath);
+	snprintf(path, sizeof(path),"/local/domain/%i/name", domain_id);
 
 	return xs_read(handle->xshandle, XBT_NULL, path, NULL);
 }
-- 
1.7.6.4

Re: [PATCH 2/2] xenstat: Use local domain names

[Ian Campbell]

On Wed, 2011-11-09 at 18:11 +0000, Daniel De Graaf wrote:
> The domain name stored in /local/domain/$domid/name is simpler to
> access and is the only domain name modified by "xl rename". Use this
> domain name in libxenstat's reporting.

As it happens I noticed this today too since it causes dom0 not to show
up in xentop. My workaround was to add to my initscripts:

xenstore-write /vm/00000000-0000-0000-0000-000000000000/name "Domain-0"
xenstore-write /local/domain/0/vm /vm/00000000-0000-0000-0000-000000000000

I wondered what xend does and it seems that it also creates
both /local/domain/N/name and /vm/UUID/name but only updates the latter
on "xm rename". So it seems that xend and xl behave exactly opposite wrt
which one they change on rename. Leaving aside that it seems buggy to a)
record the name twice and b) only update one copy on rename I think "xl
rename" should either do the same thing as "xm rename" or it should
update both names in xenstore (or maybe we should drop one).

Ian.

> 
> Signed-off-by: Daniel De Graaf <dgdegra@tycho.nsa.gov>
> ---
>  tools/xenstat/libxenstat/src/xenstat.c |   12 ++----------
>  1 files changed, 2 insertions(+), 10 deletions(-)
> 
> diff --git a/tools/xenstat/libxenstat/src/xenstat.c b/tools/xenstat/libxenstat/src/xenstat.c
> index 2791cc1..104655d 100644
> --- a/tools/xenstat/libxenstat/src/xenstat.c
> +++ b/tools/xenstat/libxenstat/src/xenstat.c
> @@ -739,17 +739,9 @@ unsigned long long xenstat_tmem_succ_pers_gets(xenstat_tmem *tmem)
>  
>  static char *xenstat_get_domain_name(xenstat_handle *handle, unsigned int domain_id)
>  {
> -	char path[80], *vmpath;
> +	char path[80];
>  
> -	snprintf(path, sizeof(path),"/local/domain/%i/vm", domain_id);
> -
> -	vmpath = xs_read(handle->xshandle, XBT_NULL, path, NULL);
> -
> -	if (vmpath == NULL)
> -		return NULL;
> -
> -	snprintf(path, sizeof(path),"%s/name", vmpath);
> -	free(vmpath);
> +	snprintf(path, sizeof(path),"/local/domain/%i/name", domain_id);
>  
>  	return xs_read(handle->xshandle, XBT_NULL, path, NULL);
>  }

Re: [PATCH 2/2] xenstat: Use local domain names

[Ian Jackson]

Ian Campbell writes ("Re: [Xen-devel] [PATCH 2/2] xenstat: Use local domain names"):
> I wondered what xend does and it seems that it also creates
> both /local/domain/N/name and /vm/UUID/name but only updates the latter
> on "xm rename". So it seems that xend and xl behave exactly opposite wrt
> which one they change on rename. Leaving aside that it seems buggy to a)
> record the name twice and b) only update one copy on rename I think "xl
> rename" should either do the same thing as "xm rename" or it should
> update both names in xenstore (or maybe we should drop one).

I think libxl should store the name only in /local/domain/$domid.
In libxl we identify domains by domid, in general, not uuid.

Since libxl is becoming the preferred toolstack I think we should
change xenstat to look in the libxl location - ie, apply Daniel's
2/2.  But I'll wait for further comments.

Ian.

Xentop - Domain-0 statistics

[Kaushik Kumar Ram]

Hi Folks,

Recently, I noticed that xentop stopped showing Domain-0's CPU utilization and other stats. 

First, is that the intended behavior for some reason? Assuming its not, am I the only one observing this issue?

I dug into the source code a bit and traced the problem to the xenstat_get_domain_name() function, which essentially finds the name of a domain by reading some values from xenstore.  But this function fails for Domain-0, and therefore xenstat_get_node() ignore Domain-0's stats.
In particular, "/local/domain/0/vm" does not exist for Domain-0 while it does for all other domains. 

I am not an expert on what/how values are stored in xenstore. But is there a reason why such a convoluted logic is used to determine a domain's name? Why not just read the value in "/local/domain/domain_id/name"?

I did exactly that and now I can see Domain-0's stats in xentop.

Is this a bug? If so, I can submit a patch.

Thanks.

--Kaushik

Re: Xentop - Domain-0 statistics

[Ian Campbell]

On Thu, 2012-03-08 at 18:06 -0500, Kaushik Kumar Ram wrote:
> Hi Folks,
> 
> Recently, I noticed that xentop stopped showing Domain-0's CPU
> utilization and other stats. 
> 
> First, is that the intended behavior for some reason? Assuming its
> not, am I the only one observing this issue?
> 
> I dug into the source code a bit and traced the problem to the
> xenstat_get_domain_name() function, which essentially finds the name
> of a domain by reading some values from xenstore.  But this function
> fails for Domain-0, and therefore xenstat_get_node() ignore Domain-0's
> stats.
> In particular, "/local/domain/0/vm" does not exist for Domain-0 while
> it does for all other domains. 
> 
> I am not an expert on what/how values are stored in xenstore. But is
> there a reason why such a convoluted logic is used to determine a
> domain's name? Why not just read the value in
> "/local/domain/domain_id/name"?
> 
> I did exactly that and now I can see Domain-0's stats in xentop.
> 
> Is this a bug? If so, I can submit a patch.

I think it is a bug. It was discussed before at
http://lists.xen.org/archives/html/xen-devel/2011-11/msg00579.html

Looks like IanJ intended to apply the patch
(http://lists.xen.org/archives/html/xen-devel/2011-11/msg01486.html) but
I guess it got dropped. I've CC'd him now
> 
> Thanks.
> 
> --Kaushik
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel

Re: Xentop - Domain-0 statistics

[Ian Jackson]

Ian Campbell writes ("Re: [Xen-devel] Xentop - Domain-0 statistics"):
> Looks like IanJ intended to apply the patch
> (http://lists.xen.org/archives/html/xen-devel/2011-11/msg01486.html) but
> I guess it got dropped. I've CC'd him now

Thanks, yes.

I will apply it (assuming it still fits...)

Ian.

Re: Xentop - Domain-0 statistics [and 1 more messages]

[Ian Jackson]

Ian Campbell writes ("Re: [Xen-devel] Xentop - Domain-0 statistics"):
> Looks like IanJ intended to apply the patch
> (http://lists.xen.org/archives/html/xen-devel/2011-11/msg01486.html) but

Daniel De Graaf writes ("[Xen-devel] [PATCH 2/2] xenstat: Use local domain names"):
> The domain name stored in /local/domain/$domid/name is simpler to
> access and is the only domain name modified by "xl rename". Use this
> domain name in libxenstat's reporting.

Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>
Committed-by: Ian Jackson <ian.jackson@eu.citrix.com>

Thanks,
Ian.