[PATCH] Fix xentop on pv-ops domain0

[PATCH] Fix xentop on pv-ops domain0

[Xu, Dongxiao]

[-- Attachment #1: Type: text/plain, Size: 255 bytes --]

Xentop encounters error in pv-ops domain0, because the VBD path in sysfs changes to "/sys/devices"
Also delete the macro in xenstat_netbsd.c, because it is not used.

Signed-off-by: Dongxiao Xu <dongxiao.xu@intel.com>

Best Regards, 
-- Dongxiao


[-- Attachment #2: fix_xentop_on_pvops_dom0.patch --]
[-- Type: application/octet-stream, Size: 2477 bytes --]

Xentop encounters error in pv-ops domain0, because the VBD path in sysfs 
changes to "/sys/devices/". 
Also delete the macro in xenstat_netbsd.c, because it is not used.

Signed-off-by: Dongxiao Xu <dongxiao.xu@intel.com>

diff -r 238a148b1447 tools/xenstat/libxenstat/src/xenstat_linux.c
--- a/tools/xenstat/libxenstat/src/xenstat_linux.c	Tue Jun 30 16:00:57 2009 +0100
+++ b/tools/xenstat/libxenstat/src/xenstat_linux.c	Fri Jul 03 15:57:45 2009 +0800
@@ -31,7 +31,7 @@
 
 #include "xenstat_priv.h"
 
-#define SYSFS_VBD_PATH "/sys/devices/xen-backend/"
+char *sysfs_vbd_path;
 
 struct priv_data {
 	FILE *procnetdev;
@@ -166,7 +166,7 @@ static int read_attributes_vbd(const cha
 	int fd, num_read;
 
 	snprintf(file_name, sizeof(file_name), "%s/%s/%s",
-		SYSFS_VBD_PATH, vbd_directory, what);
+		sysfs_vbd_path, vbd_directory, what);
 	fd = open(file_name, O_RDONLY, 0);
 	if (fd==-1) return -1;
 	num_read = read(fd, ret, cap - 1);
@@ -188,9 +188,9 @@ int xenstat_collect_vbds(xenstat_node * 
 	}
 
 	if (priv->sysfsvbd == NULL) {
-		priv->sysfsvbd = opendir(SYSFS_VBD_PATH);
+		priv->sysfsvbd = opendir(sysfs_vbd_path);
 		if (priv->sysfsvbd == NULL) {
-			perror("Error opening " SYSFS_VBD_PATH);
+			perror("Error opening the sysfs vbd path");
 			return 0;
 		}
 	}
diff -r 238a148b1447 tools/xenstat/libxenstat/src/xenstat_netbsd.c
--- a/tools/xenstat/libxenstat/src/xenstat_netbsd.c	Tue Jun 30 16:00:57 2009 +0100
+++ b/tools/xenstat/libxenstat/src/xenstat_netbsd.c	Fri Jul 03 15:57:45 2009 +0800
@@ -30,8 +30,6 @@
 #include <unistd.h>
 
 #include "xenstat_priv.h"
-
-#define SYSFS_VBD_PATH "/sys/devices/xen-backend/"
 
 struct priv_data {
 	FILE *procnetdev;
diff -r 238a148b1447 tools/xenstat/xentop/xentop.c
--- a/tools/xenstat/xentop/xentop.c	Tue Jun 30 16:00:57 2009 +0100
+++ b/tools/xenstat/xentop/xentop.c	Fri Jul 03 15:57:45 2009 +0800
@@ -34,6 +34,7 @@
 #endif
 
 #include <xenstat.h>
+#include <sys/utsname.h>
 
 #define XENTOP_VERSION "1.0"
 
@@ -1038,6 +1039,17 @@ int main(int argc, char **argv)
 		{ 0, 0, 0, 0 },
 	};
 	const char *sopts = "hVnxrvd:bi:";
+	struct utsname system_info;
+	extern char *sysfs_vbd_path;
+
+	if (uname(&system_info) < 0)
+		fail("Failed to get domain0 version");
+
+	if (strncmp(system_info.release, "2.6.18.8-xen", 
+		sizeof("2.6.18.8-xen")) == 0)
+		sysfs_vbd_path = "/sys/devices/xen-backend/";
+	else
+		sysfs_vbd_path = "/sys/devices/";
 
 	if (atexit(cleanup) != 0)
 		fail("Failed to install cleanup handler.\n");

[-- Attachment #3: Type: text/plain, Size: 138 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

Re: [PATCH] Fix xentop on pv-ops domain0

[Christoph Egger]

On Monday 06 July 2009 15:08:06 Xu, Dongxiao wrote:
> Xentop encounters error in pv-ops domain0, because the VBD path in sysfs
> changes to "/sys/devices" Also delete the macro in xenstat_netbsd.c,
> because it is not used.
>
> Signed-off-by: Dongxiao Xu <dongxiao.xu@intel.com>

Can you move the Linux specific code you add in xentop.c into xenstat_linux.c, 
please? This is an abstraction violation, otherwise.

Thanks,
Christoph


-- 
---to satisfy European Law for business letters:
Advanced Micro Devices GmbH
Karl-Hammerschmidt-Str. 34, 85609 Dornach b. Muenchen
Geschaeftsfuehrer: Thomas M. McCoy, Giuliano Meroni
Sitz: Dornach, Gemeinde Aschheim, Landkreis Muenchen
Registergericht Muenchen, HRB Nr. 43632

RE: [PATCH] Fix xentop on pv-ops domain0

[Xu, Dongxiao]

[-- Attachment #1: Type: text/plain, Size: 1090 bytes --]

Thanks for comments. The revised patch is attached!

Best Regards, 
-- Dongxiao

-----Original Message-----
From: Christoph Egger [mailto:Christoph.Egger@amd.com] 
Sent: Monday, July 06, 2009 9:35 PM
To: xen-devel@lists.xensource.com
Cc: Xu, Dongxiao; keir.fraser@eu.citrix.com
Subject: Re: [Xen-devel] [PATCH] Fix xentop on pv-ops domain0

On Monday 06 July 2009 15:08:06 Xu, Dongxiao wrote:
> Xentop encounters error in pv-ops domain0, because the VBD path in sysfs
> changes to "/sys/devices" Also delete the macro in xenstat_netbsd.c,
> because it is not used.
>
> Signed-off-by: Dongxiao Xu <dongxiao.xu@intel.com>

Can you move the Linux specific code you add in xentop.c into xenstat_linux.c, 
please? This is an abstraction violation, otherwise.

Thanks,
Christoph


-- 
---to satisfy European Law for business letters:
Advanced Micro Devices GmbH
Karl-Hammerschmidt-Str. 34, 85609 Dornach b. Muenchen
Geschaeftsfuehrer: Thomas M. McCoy, Giuliano Meroni
Sitz: Dornach, Gemeinde Aschheim, Landkreis Muenchen
Registergericht Muenchen, HRB Nr. 43632


[-- Attachment #2: fix_xentop_on_pvops_dom0.patch --]
[-- Type: application/octet-stream, Size: 2271 bytes --]

Xentop encounters error in pv-ops domain0, because the VBD path in sysfs 
changes to "/sys/devices"
Also delete the macro in xenstat_netbsd.c, because it is not used.

Signed-off-by: Dongxiao Xu <dongxiao.xu@intel.com>

diff -r 238a148b1447 tools/xenstat/libxenstat/src/xenstat_linux.c
--- a/tools/xenstat/libxenstat/src/xenstat_linux.c	Tue Jun 30 16:00:57 2009 +0100
+++ b/tools/xenstat/libxenstat/src/xenstat_linux.c	Mon Jul 06 21:51:19 2009 +0800
@@ -28,10 +28,11 @@
 #include <stdlib.h>
 #include <string.h>
 #include <unistd.h>
+#include <sys/utsname.h>
 
 #include "xenstat_priv.h"
 
-#define SYSFS_VBD_PATH "/sys/devices/xen-backend/"
+static char *sysfs_vbd_path;
 
 struct priv_data {
 	FILE *procnetdev;
@@ -166,7 +167,7 @@ static int read_attributes_vbd(const cha
 	int fd, num_read;
 
 	snprintf(file_name, sizeof(file_name), "%s/%s/%s",
-		SYSFS_VBD_PATH, vbd_directory, what);
+		sysfs_vbd_path, vbd_directory, what);
 	fd = open(file_name, O_RDONLY, 0);
 	if (fd==-1) return -1;
 	num_read = read(fd, ret, cap - 1);
@@ -181,6 +182,19 @@ int xenstat_collect_vbds(xenstat_node * 
 {
 	struct dirent *dp;
 	struct priv_data *priv = get_priv_data(node->handle);
+	struct utsname system_info;
+
+	if (uname(&system_info) < 0) {
+		perror("Failed to get domain0 version");
+		return 0;
+	}
+
+	if (strncmp(system_info.release, "2.6.18.8-xen", 
+		sizeof("2.6.18.8-xen")) == 0)
+		sysfs_vbd_path = "/sys/devices/xen-backend/";
+	else
+		sysfs_vbd_path = "/sys/devices/";
+
 
 	if (priv == NULL) {
 		perror("Allocation error");
@@ -188,9 +202,9 @@ int xenstat_collect_vbds(xenstat_node * 
 	}
 
 	if (priv->sysfsvbd == NULL) {
-		priv->sysfsvbd = opendir(SYSFS_VBD_PATH);
+		priv->sysfsvbd = opendir(sysfs_vbd_path);
 		if (priv->sysfsvbd == NULL) {
-			perror("Error opening " SYSFS_VBD_PATH);
+			perror("Error opening the sysfs vbd path");
 			return 0;
 		}
 	}
diff -r 238a148b1447 tools/xenstat/libxenstat/src/xenstat_netbsd.c
--- a/tools/xenstat/libxenstat/src/xenstat_netbsd.c	Tue Jun 30 16:00:57 2009 +0100
+++ b/tools/xenstat/libxenstat/src/xenstat_netbsd.c	Mon Jul 06 21:51:19 2009 +0800
@@ -30,8 +30,6 @@
 #include <unistd.h>
 
 #include "xenstat_priv.h"
-
-#define SYSFS_VBD_PATH "/sys/devices/xen-backend/"
 
 struct priv_data {
 	FILE *procnetdev;

[-- Attachment #3: Type: text/plain, Size: 138 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

Re: [PATCH] Fix xentop on pv-ops domain0

[Christoph Egger]

This patch is good.

Christoph


On Monday 06 July 2009 16:08:10 Xu, Dongxiao wrote:
> Thanks for comments. The revised patch is attached!
>
> Best Regards,
> -- Dongxiao
>
> -----Original Message-----
> From: Christoph Egger [mailto:Christoph.Egger@amd.com]
> Sent: Monday, July 06, 2009 9:35 PM
> To: xen-devel@lists.xensource.com
> Cc: Xu, Dongxiao; keir.fraser@eu.citrix.com
> Subject: Re: [Xen-devel] [PATCH] Fix xentop on pv-ops domain0
>
> On Monday 06 July 2009 15:08:06 Xu, Dongxiao wrote:
> > Xentop encounters error in pv-ops domain0, because the VBD path in sysfs
> > changes to "/sys/devices" Also delete the macro in xenstat_netbsd.c,
> > because it is not used.
> >
> > Signed-off-by: Dongxiao Xu <dongxiao.xu@intel.com>
>
> Can you move the Linux specific code you add in xentop.c into
> xenstat_linux.c, please? This is an abstraction violation, otherwise.
>
> Thanks,
> Christoph



-- 
---to satisfy European Law for business letters:
Advanced Micro Devices GmbH
Karl-Hammerschmidt-Str. 34, 85609 Dornach b. Muenchen
Geschaeftsfuehrer: Thomas M. McCoy, Giuliano Meroni
Sitz: Dornach, Gemeinde Aschheim, Landkreis Muenchen
Registergericht Muenchen, HRB Nr. 43632

Re: [PATCH] Fix xentop on pv-ops domain0

[Tim Post]

Hi,

On Mon, 2009-07-06 at 17:49 +0200, Christoph Egger wrote:
> This patch is good.
> 

The following is going to break xenstat on almost every forward port of
2.6.18.8 (including Andy's 2.6.30 vanilla port):


+       if (strncmp(system_info.release, "2.6.18.8-xen", 
+               sizeof("2.6.18.8-xen")) == 0)
+               sysfs_vbd_path = "/sys/devices/xen-backend/";
+       else
+               sysfs_vbd_path = "/sys/devices/";
+

I would really recommend considering just
using /sys/bus/xen-backend/devices, as Stephen noted.

Cheers,
--Tim

RE: [PATCH] Fix xentop on pv-ops domain0

[Xu, Dongxiao]

Using "/sys/bus/xen-backend/devices/" is fine, formerly I didn't notice there was a link in this directory. Thanks!

Best Regards, 
-- Dongxiao

-----Original Message-----
From: Tim Post [mailto:echo@echoreply.us] 
Sent: Tuesday, July 07, 2009 12:02 AM
To: Christoph Egger
Cc: Xu, Dongxiao; xen-devel@lists.xensource.com; keir.fraser@eu.citrix.com
Subject: Re: [Xen-devel] [PATCH] Fix xentop on pv-ops domain0

Hi,

On Mon, 2009-07-06 at 17:49 +0200, Christoph Egger wrote:
> This patch is good.
> 

The following is going to break xenstat on almost every forward port of
2.6.18.8 (including Andy's 2.6.30 vanilla port):


+       if (strncmp(system_info.release, "2.6.18.8-xen", 
+               sizeof("2.6.18.8-xen")) == 0)
+               sysfs_vbd_path = "/sys/devices/xen-backend/";
+       else
+               sysfs_vbd_path = "/sys/devices/";
+

I would really recommend considering just
using /sys/bus/xen-backend/devices, as Stephen noted.

Cheers,
--Tim

Re: [PATCH] Fix xentop on pv-ops domain0

[Keir Fraser]

Please send a final patch.

 Thanks,
 Keir

On 07/07/2009 01:37, "Xu, Dongxiao" <dongxiao.xu@intel.com> wrote:

> Using "/sys/bus/xen-backend/devices/" is fine, formerly I didn't notice there
> was a link in this directory. Thanks!
> 
> Best Regards, 
> -- Dongxiao
> 
> -----Original Message-----
> From: Tim Post [mailto:echo@echoreply.us]
> Sent: Tuesday, July 07, 2009 12:02 AM
> To: Christoph Egger
> Cc: Xu, Dongxiao; xen-devel@lists.xensource.com; keir.fraser@eu.citrix.com
> Subject: Re: [Xen-devel] [PATCH] Fix xentop on pv-ops domain0
> 
> Hi,
> 
> On Mon, 2009-07-06 at 17:49 +0200, Christoph Egger wrote:
>> This patch is good.
>> 
> 
> The following is going to break xenstat on almost every forward port of
> 2.6.18.8 (including Andy's 2.6.30 vanilla port):
> 
> 
> +       if (strncmp(system_info.release, "2.6.18.8-xen",
> +               sizeof("2.6.18.8-xen")) == 0)
> +               sysfs_vbd_path = "/sys/devices/xen-backend/";
> +       else
> +               sysfs_vbd_path = "/sys/devices/";
> +
> 
> I would really recommend considering just
> using /sys/bus/xen-backend/devices, as Stephen noted.
> 
> Cheers,
> --Tim
> 
> 

RE: [PATCH] Fix xentop on pv-ops domain0

[Xu, Dongxiao]

Hi, Keir, 
	The following patch is from Ian Campbell, sent at Fri 5/8/2009 5:03 PM. 



This path doesn't exist in the pvops kernel. Looks
like /sys/bus/xen-backend/devices is just as good and exists in both
pvops and 2.6.18 kernels.

Subject: xenstat: Use backend path which is compatible with pvops and 2.6.18-xen kernels.

Signed-off-by: Ian Campbell <ian.campbell@citrix.com>

diff -r 07812e857e67 tools/xenstat/libxenstat/src/xenstat_linux.c
--- a/tools/xenstat/libxenstat/src/xenstat_linux.c	Thu Apr 09 12:09:14 2009 +0100
+++ b/tools/xenstat/libxenstat/src/xenstat_linux.c	Fri May 08 09:40:45 2009 +0100
@@ -31,7 +31,7 @@
 
 #include "xenstat_priv.h"
 
-#define SYSFS_VBD_PATH "/sys/devices/xen-backend/"
+#define SYSFS_VBD_PATH "/sys/bus/xen-backend/devices"
 
 struct priv_data {
 	FILE *procnetdev;

Best Regards, 
-- Dongxiao

-----Original Message-----
From: Keir Fraser [mailto:keir.fraser@eu.citrix.com] 
Sent: Tuesday, July 07, 2009 3:16 PM
To: Xu, Dongxiao; echo@echoreply.us; Christoph Egger; Steven Maresca
Cc: xen-devel@lists.xensource.com
Subject: Re: [Xen-devel] [PATCH] Fix xentop on pv-ops domain0

Please send a final patch.

 Thanks,
 Keir

On 07/07/2009 01:37, "Xu, Dongxiao" <dongxiao.xu@intel.com> wrote:

> Using "/sys/bus/xen-backend/devices/" is fine, formerly I didn't notice there
> was a link in this directory. Thanks!
> 
> Best Regards, 
> -- Dongxiao
> 
> -----Original Message-----
> From: Tim Post [mailto:echo@echoreply.us]
> Sent: Tuesday, July 07, 2009 12:02 AM
> To: Christoph Egger
> Cc: Xu, Dongxiao; xen-devel@lists.xensource.com; keir.fraser@eu.citrix.com
> Subject: Re: [Xen-devel] [PATCH] Fix xentop on pv-ops domain0
> 
> Hi,
> 
> On Mon, 2009-07-06 at 17:49 +0200, Christoph Egger wrote:
>> This patch is good.
>> 
> 
> The following is going to break xenstat on almost every forward port of
> 2.6.18.8 (including Andy's 2.6.30 vanilla port):
> 
> 
> +       if (strncmp(system_info.release, "2.6.18.8-xen",
> +               sizeof("2.6.18.8-xen")) == 0)
> +               sysfs_vbd_path = "/sys/devices/xen-backend/";
> +       else
> +               sysfs_vbd_path = "/sys/devices/";
> +
> 
> I would really recommend considering just
> using /sys/bus/xen-backend/devices, as Stephen noted.
> 
> Cheers,
> --Tim
> 
> 

RE: [PATCH] Fix xentop on pv-ops domain0

[Steven Maresca]

Hello,

>Xentop encounters error in pv-ops domain0, because the VBD path in sysfs changes to "/sys/devices"
>Also delete the macro in xenstat_netbsd.c, because it is not used.
>
>Signed-off-by: Dongxiao Xu <dongxiao.xu@intel.com>
>
>Best Regards,
>-- Dongxiao

The patch you've provided explicitly checks for a .18-xen kernel;
doing so runs the risk of breaking behavior on every forward port
after .18.   This is quite fragile.

It is my suggestion that, instead, we might utilize the patch Ian
Campbell suggested some time ago. See
http://lists.xensource.com/archives/html/xen-devel/2009-05/msg00299.html

That patch is a small tweak: #define SYSFS_VBD_PATH
"/sys/bus/xen-backend/devices"
Luckily, this change satisfies legacy .18-xen, pvops, (and even older
.16-xen kernels),

Xenstat - while useful - is brittle enough as it is, so keeping things
generic would be best.

-Steve Maresca