[PATCH v3] xentop: add support for qdisks

[PATCH v3] xentop: add support for qdisks

[Charles Arnold]

Now that Xen uses qdisks by default and qemu does not write out
statistics to sysfs this patch queries the QMP for disk statistics.

This patch depends on libyajl for parsing statistics returned from
QMP. The runtime requires libyajl 2.0.3 or newer for required bug
fixes in yajl_tree_parse().

Libxl is modified to create a new socket dedicated for the use of
libxenstat for querying the block statistics using QMP.

The current APIs remain unchanged. It works within the existing
framework of libxenstat.

Signed-off-by: Charles Arnold <carnold@suse.com>

diff --git a/tools/libxl/libxl_dm.c b/tools/libxl/libxl_dm.c
index a8b08f2..d72cb1d 100644
--- a/tools/libxl/libxl_dm.c
+++ b/tools/libxl/libxl_dm.c
@@ -454,6 +454,15 @@ static char ** libxl__build_device_model_args_new(libxl__gc *gc,
     flexarray_append(dm_args, "-mon");
     flexarray_append(dm_args, "chardev=libxl-cmd,mode=control");
 
+    flexarray_append(dm_args, "-chardev");
+    flexarray_append(dm_args,
+                     libxl__sprintf(gc, "socket,id=libxenstat-cmd,"
+                                    "path=%s/qmp-libxenstat-%d,server,nowait",
+                                    libxl__run_dir_path(), guest_domid));
+
+    flexarray_append(dm_args, "-mon");
+    flexarray_append(dm_args, "chardev=libxenstat-cmd,mode=control");
+
     for (i = 0; i < guest_config->num_channels; i++) {
         connection = guest_config->channels[i].connection;
         devid = guest_config->channels[i].devid;
diff --git a/tools/libxl/libxl_qmp.c b/tools/libxl/libxl_qmp.c
index c7324e6..7f3731b 100644
--- a/tools/libxl/libxl_qmp.c
+++ b/tools/libxl/libxl_qmp.c
@@ -723,6 +723,13 @@ void libxl__qmp_cleanup(libxl__gc *gc, uint32_t domid)
             LOGE(ERROR, "Failed to remove QMP socket file %s", qmp_socket);
         }
     }
+
+    qmp_socket = GCSPRINTF("%s/qmp-libxenstat-%d", libxl__run_dir_path(), domid);
+    if (unlink(qmp_socket) == -1) {
+        if (errno != ENOENT) {
+            LOGE(ERROR, "Failed to remove QMP socket file %s", qmp_socket);
+        }
+    }
 }
 
 int libxl__qmp_query_serial(libxl__qmp_handler *qmp)
diff --git a/tools/xenstat/libxenstat/Makefile b/tools/xenstat/libxenstat/Makefile
index 07d39b1..7c973f8 100644
--- a/tools/xenstat/libxenstat/Makefile
+++ b/tools/xenstat/libxenstat/Makefile
@@ -24,7 +24,7 @@ MINOR=0
 LIB=src/libxenstat.a
 SHLIB=src/libxenstat.so.$(MAJOR).$(MINOR)
 SHLIB_LINKS=src/libxenstat.so.$(MAJOR) src/libxenstat.so
-OBJECTS-y=src/xenstat.o
+OBJECTS-y=src/xenstat.o src/xenstat_qmp.o
 OBJECTS-$(CONFIG_Linux) += src/xenstat_linux.o
 OBJECTS-$(CONFIG_SunOS) += src/xenstat_solaris.o
 OBJECTS-$(CONFIG_NetBSD) += src/xenstat_netbsd.o
@@ -32,7 +32,7 @@ OBJECTS-$(CONFIG_FreeBSD) += src/xenstat_freebsd.o
 SONAME_FLAGS=-Wl,$(SONAME_LDFLAG) -Wl,libxenstat.so.$(MAJOR)
 
 CFLAGS+=-fPIC
-CFLAGS+=-Isrc $(CFLAGS_libxenctrl) $(CFLAGS_libxenstore) $(CFLAGS_xeninclude)
+CFLAGS+=-Isrc $(CFLAGS_libxenctrl) $(CFLAGS_libxenstore) $(CFLAGS_xeninclude) -include $(XEN_ROOT)/tools/config.h
 
 LDLIBS-y = $(LDLIBS_libxenstore) $(LDLIBS_libxenctrl)
 LDLIBS-$(CONFIG_SunOS) += -lkstat
diff --git a/tools/xenstat/libxenstat/src/xenstat.c b/tools/xenstat/libxenstat/src/xenstat.c
index 8072a90..3ad89c4 100644
--- a/tools/xenstat/libxenstat/src/xenstat.c
+++ b/tools/xenstat/libxenstat/src/xenstat.c
@@ -657,6 +657,27 @@ static void xenstat_uninit_xen_version(xenstat_handle * handle)
  * VBD functions
  */
 
+/* Save VBD information */
+xenstat_vbd *xenstat_save_vbd(xenstat_domain *domain, xenstat_vbd *vbd)
+{
+        xenstat_vbd *vbds = domain->vbds;
+
+        domain->num_vbds++;
+        domain->vbds = realloc(domain->vbds,
+                               domain->num_vbds *
+                               sizeof(xenstat_vbd));
+
+        if (domain->vbds == NULL) {
+                domain->num_vbds = 0;
+                free(vbds);
+        }
+        else {
+                domain->vbds[domain->num_vbds - 1] = *vbd;
+        }
+
+        return domain->vbds;
+}
+
 /* Free VBD information */
 static void xenstat_free_vbds(xenstat_node * node)
 {
diff --git a/tools/xenstat/libxenstat/src/xenstat_linux.c b/tools/xenstat/libxenstat/src/xenstat_linux.c
index 7fdf70a..2cc9c7f 100644
--- a/tools/xenstat/libxenstat/src/xenstat_linux.c
+++ b/tools/xenstat/libxenstat/src/xenstat_linux.c
@@ -417,6 +417,9 @@ int xenstat_collect_vbds(xenstat_node * node)
 		}
 	}
 
+	/* Get qdisk statistics */
+	read_attributes_qdisk(node);
+
 	rewinddir(priv->sysfsvbd);
 
 	for(dp = readdir(priv->sysfsvbd); dp != NULL ;
@@ -477,18 +480,10 @@ int xenstat_collect_vbds(xenstat_node * node)
 			continue;
 		}
 
-		if (domain->vbds == NULL) {
-			domain->num_vbds = 1;
-			domain->vbds = malloc(sizeof(xenstat_vbd));
-		} else {
-			domain->num_vbds++;
-			domain->vbds = realloc(domain->vbds,
-					       domain->num_vbds *
-					       sizeof(xenstat_vbd));
-		}
-		if (domain->vbds == NULL)
+		if ((xenstat_save_vbd(domain, &vbd)) == NULL) {
+			perror("Allocation error");
 			return 0;
-		domain->vbds[domain->num_vbds - 1] = vbd;
+		}
 	}
 
 	return 1;	
diff --git a/tools/xenstat/libxenstat/src/xenstat_priv.h b/tools/xenstat/libxenstat/src/xenstat_priv.h
index 8490e23..74e0774 100644
--- a/tools/xenstat/libxenstat/src/xenstat_priv.h
+++ b/tools/xenstat/libxenstat/src/xenstat_priv.h
@@ -109,5 +109,7 @@ extern int xenstat_collect_networks(xenstat_node * node);
 extern void xenstat_uninit_networks(xenstat_handle * handle);
 extern int xenstat_collect_vbds(xenstat_node * node);
 extern void xenstat_uninit_vbds(xenstat_handle * handle);
+extern void read_attributes_qdisk(xenstat_node * node);
+extern xenstat_vbd *xenstat_save_vbd(xenstat_domain * domain, xenstat_vbd * vbd);
 
 #endif /* XENSTAT_PRIV_H */
diff --git a/tools/xenstat/xentop/Makefile b/tools/xenstat/xentop/Makefile
index e2665aa..5376fdc 100644
--- a/tools/xenstat/xentop/Makefile
+++ b/tools/xenstat/xentop/Makefile
@@ -19,7 +19,7 @@ all install xentop:
 else
 
 CFLAGS += -DGCC_PRINTF -Werror $(CFLAGS_libxenstat)
-LDLIBS += $(LDLIBS_libxenstat) $(CURSES_LIBS) $(SOCKET_LIBS) -lm
+LDLIBS += $(LDLIBS_libxenstat) $(CURSES_LIBS) $(SOCKET_LIBS) -lm -lyajl
 CFLAGS += -DHOST_$(XEN_OS)
 
 # Include configure output (config.h) to headers search path
diff --git a/tools/xenstat/libxenstat/src/xenstat_qmp.c b/tools/xenstat/libxenstat/src/xenstat_qmp.c
new file mode 100644
index 0000000..c217b8e
--- /dev/null
+++ b/tools/xenstat/libxenstat/src/xenstat_qmp.c
@@ -0,0 +1,450 @@
+/* libxenstat: statistics-collection library for Xen
+ *
+ * This library is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public
+ * License as published by the Free Software Foundation; either
+ * version 2.1 of the License, or (at your option) any later version.
+ *
+ * This library is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * Lesser General Public License for more details.
+ */
+
+#include <fcntl.h>
+#include <sys/types.h>
+#include <sys/socket.h>
+#include <sys/poll.h>
+#include <sys/un.h>
+#include <stdlib.h>
+#include <string.h>
+#include <unistd.h>
+
+#include <xenctrl.h>
+
+#include "xenstat_priv.h"
+
+#ifdef HAVE_YAJL_YAJL_VERSION_H
+#  include <yajl/yajl_version.h>
+#endif
+
+/* YAJL version check */
+#if defined(YAJL_MAJOR) && (YAJL_MAJOR > 1)
+#  define HAVE_YAJL_V2 1
+#endif
+
+#ifdef HAVE_YAJL_V2
+
+#include <yajl/yajl_tree.h>
+
+static unsigned char *qmp_query(int, char *);
+
+enum query_blockstats {
+    QMP_STATS_RETURN  = 0,
+    QMP_STATS_DEVICE  = 1,
+    QMP_STATS         = 2,
+    QMP_RD_BYTES      = 3,
+    QMP_WR_BYTES      = 4,
+    QMP_RD_OPERATIONS = 5,
+    QMP_WR_OPERATIONS = 6,
+};
+
+enum query_block {
+    QMP_BLOCK_RETURN  = 0,
+    QMP_BLOCK_DEVICE  = 1,
+    QMP_INSERTED      = 2,
+    QMP_FILE          = 3,
+};
+
+
+/* Given the qmp device name, get the image filename associated with it
+   QMP Syntax for querying block infomation:
+     In: { "execute": "query-block" }
+     Out: {"return": [{
+            "device": 'str, "locked": 'bool', "removable": bool,
+            "inserted": {
+              "iops_rd": 'int',
+              "image": {
+                "virtual-size": 'int', "filename": 'str', "cluster-size": 'int',
+                "format": 'str', "actual-size": 'int', "dirty-flag": 'bool'
+              },
+              "iops_wr": 'int', "ro": 'bool', "backing_file_depth": 'int',
+              "drv": 'str', "iops": 'int', "bps_wr": 'int', "encrypted": 'bool',
+              "bps": 'int', "bps_rd": 'int',
+              "file": 'str', "encryption_key_missing": 'bool'
+            },
+            "type": 'str'
+          }]}
+*/
+static char *qmp_get_block_image(xenstat_node *node, char *qmp_devname, int qfd)
+{
+	char *tmp, *file = NULL;
+	char *query_block_cmd = "{ \"execute\": \"query-block\" }";
+	static const char *const qblock[] = {
+		[ QMP_BLOCK_RETURN  ] = "return",
+		[ QMP_BLOCK_DEVICE  ] = "device",
+		[ QMP_INSERTED      ] = "inserted",
+		[ QMP_FILE          ] = "file",
+	};
+	const char *ptr[] = {0, 0};
+	unsigned char *qmp_stats;
+	yajl_val info, ret_obj, dev_obj, n;
+	int i;
+
+	if ((qmp_stats = qmp_query(qfd, query_block_cmd)) == NULL)
+		return NULL;
+
+	/* Use libyajl version 2.0.3 or newer for the tree parser feature with bug fixes */
+	if ((info = yajl_tree_parse((char *)qmp_stats, NULL, 0)) == NULL) {
+		free(qmp_stats);
+		return NULL;
+	}
+
+	ptr[0] = qblock[QMP_BLOCK_RETURN]; /* "return" */
+	if ((ret_obj = yajl_tree_get(info, ptr, yajl_t_array)) == NULL)
+		goto done;
+
+	for (i=0; i<YAJL_GET_ARRAY(ret_obj)->len; i++) {
+		n = YAJL_GET_ARRAY(ret_obj)->values[i];
+
+		ptr[0] = qblock[QMP_BLOCK_DEVICE]; /* "device" */
+		if ((dev_obj = yajl_tree_get(n, ptr, yajl_t_any)) != NULL) {
+			tmp = YAJL_GET_STRING(dev_obj);
+			if (strcmp(qmp_devname, tmp))
+				continue;
+		}
+		else
+			continue;
+
+		ptr[0] = qblock[QMP_INSERTED]; /* "inserted" */
+		n = yajl_tree_get(n, ptr, yajl_t_any);
+		if (n) {
+			ptr[0] = qblock[QMP_FILE]; /* "file" */
+			n = yajl_tree_get(n, ptr, yajl_t_any);
+			if (n && YAJL_IS_STRING(n)) {
+				tmp = YAJL_GET_STRING(n);
+				file = malloc(strlen(tmp)+1);
+				if (file != NULL)
+					strcpy(file, tmp);
+				goto done;
+			}
+		}
+	}
+done:
+	yajl_tree_free(info);
+	return file;
+}
+
+
+/* Given a QMP device name, lookup the associated xenstore qdisk device id */
+static void lookup_xenstore_devid(xenstat_node * node, unsigned int domid, char *qmp_devname,
+	int qfd, unsigned int *dev, unsigned int *sector_size)
+{
+	char **dev_ids, *tmp, *ptr, *image, path[80];
+	unsigned int num_dev_ids;
+	int i, devid;
+
+	/* Get all the qdisk dev IDs associated with the this VM */
+	snprintf(path, sizeof(path),"/local/domain/0/backend/qdisk/%i", domid);
+	dev_ids = xs_directory(node->handle->xshandle, XBT_NULL, path, &num_dev_ids);
+	if (dev_ids == NULL) {
+		return;
+	}
+
+	/* Get the filename of the image associated with this QMP device */
+	image = qmp_get_block_image(node, qmp_devname, qfd);
+	if (image == NULL) {
+		free(dev_ids);
+		return;
+	}
+
+	/* Look for a matching image in xenstore */
+	for (i=0; i<num_dev_ids; i++) {
+		devid = atoi(dev_ids[i]);
+		/* Get the xenstore name of the image */
+		snprintf(path, sizeof(path),"/local/domain/0/backend/qdisk/%i/%i/params", domid, devid);
+		if ((ptr = xs_read(node->handle->xshandle, XBT_NULL, path, NULL)) == NULL)
+			continue;
+
+		/* Get to actual path in string */
+		if ((tmp = strchr(ptr, '/')) == NULL)
+			tmp = ptr;
+		if (!strcmp(tmp,image)) {
+			*dev = devid;
+			free(ptr);
+
+			/* Get the xenstore sector size of the image while we're here */
+			snprintf(path, sizeof(path),"/local/domain/0/backend/qdisk/%i/%i/sector-size", domid, devid);
+			if ((ptr = xs_read(node->handle->xshandle, XBT_NULL, path, NULL)) != NULL) {
+				*sector_size = atoi((char *)ptr);
+				free(ptr);
+			}
+			break;
+		}
+		free(ptr);
+	}
+
+	free(image);
+	free(dev_ids);
+}
+
+/* Parse the stats buffer which contains I/O data for all the disks belonging to domid */
+static void qmp_parse_stats(xenstat_node *node, unsigned int domid, unsigned char *stats_buf, int qfd)
+{
+	char *qmp_devname;
+	static const char *const qstats[] = {
+		[ QMP_STATS_RETURN  ] = "return",
+		[ QMP_STATS_DEVICE  ] = "device",
+		[ QMP_STATS         ] = "stats",
+		[ QMP_RD_BYTES      ] = "rd_bytes",
+		[ QMP_WR_BYTES      ] = "wr_bytes",
+		[ QMP_RD_OPERATIONS ] = "rd_operations",
+		[ QMP_WR_OPERATIONS ] = "wr_operations",
+	};
+	const char *ptr[] = {0, 0};
+	yajl_val info, ret_obj, stats_obj, n;
+	xenstat_vbd vbd;
+	xenstat_domain *domain;
+	unsigned int sector_size = 512;
+	int i, j;
+
+	/* Use libyajl version 2.0.3 or newer for the tree parser feature */
+	if ((info = yajl_tree_parse((char *)stats_buf, NULL, 0)) == NULL)
+		return;
+
+	ptr[0] = qstats[QMP_STATS_RETURN]; /* "return" */
+	if ((ret_obj = yajl_tree_get(info, ptr, yajl_t_array)) == NULL)
+		goto done;
+
+	/* Array of devices */
+	for (i=0; i<YAJL_GET_ARRAY(ret_obj)->len; i++) {
+		memset(&vbd, 0, sizeof(xenstat_vbd));
+		qmp_devname = NULL;
+		stats_obj = YAJL_GET_ARRAY(ret_obj)->values[i];
+
+		ptr[0] = qstats[QMP_STATS_DEVICE]; /* "device" */
+		if ((n = yajl_tree_get(stats_obj, ptr, yajl_t_any)) != NULL)
+			qmp_devname = YAJL_GET_STRING(n);
+
+		ptr[0] = qstats[QMP_STATS]; /* "stats" */
+		stats_obj = yajl_tree_get(stats_obj, ptr, yajl_t_object);
+		if (stats_obj && YAJL_IS_OBJECT(stats_obj)) {
+			for (j=3; j<7; j++) {
+				ptr[0] = qstats[j];
+				n = yajl_tree_get(stats_obj, ptr, yajl_t_number);
+				if (n && YAJL_IS_NUMBER(n)) {
+					switch(j) {
+					case QMP_RD_BYTES: /* "rd_bytes" */
+						vbd.rd_sects = YAJL_GET_INTEGER(n) / sector_size;
+						break;
+					case QMP_WR_BYTES: /* "wr_bytes" */
+						vbd.wr_sects = YAJL_GET_INTEGER(n) / sector_size;
+						break;
+					case QMP_RD_OPERATIONS: /* "rd_operations" */
+						vbd.rd_reqs = YAJL_GET_INTEGER(n);
+						break;
+					case QMP_WR_OPERATIONS: /* "wr_operations" */
+						vbd.wr_reqs = YAJL_GET_INTEGER(n);
+						break;
+					}
+				}
+			}
+			/* With the QMP device name, lookup the xenstore qdisk device ID and set vdb.dev */
+			if (qmp_devname)
+				lookup_xenstore_devid(node, domid, qmp_devname, qfd, &vbd.dev, &sector_size);
+			if ((domain = xenstat_node_domain(node, domid)) == NULL)
+				continue;
+			if ((xenstat_save_vbd(domain, &vbd)) == NULL)
+				goto done;
+		}
+	}
+done:
+	yajl_tree_free(info);
+}
+
+/* Write a command via the QMP. Returns number of bytes written */
+static size_t qmp_write(int qfd, char *cmd, size_t cmd_len)
+{
+	size_t pos = 0;
+	ssize_t res;
+
+	while (cmd_len > pos) {
+		res = write(qfd, cmd + pos, cmd_len - pos);
+		switch (res) {
+		case -1:
+			if (errno == EINTR || errno == EAGAIN)
+				continue;
+			return 0;
+		case 0:
+			errno = EPIPE;
+			return pos;
+		default:
+			pos += (size_t)res;
+		}
+	}
+	return pos;
+}
+
+/* Read the data sent in response to a QMP execute query. Returns 1 for success */
+static int qmp_read(int qfd, unsigned char **qstats)
+{
+	unsigned char buf[1024], *ptr;
+	struct pollfd pfd[2];
+	int n, qsize = 0;
+
+	*qstats = NULL;
+	pfd[0].fd = qfd;
+	pfd[0].events = POLLIN;
+	while ((n = poll(pfd, POLLIN, 10)) > 0) {
+		if (pfd[0].revents & POLLIN) {
+			if ((n = read(qfd, buf, sizeof(buf))) < 0) {
+				return 0;
+			}
+			ptr = realloc(*qstats, qsize+n+1);
+			if (ptr == NULL) {
+				free(*qstats);
+				return 0;
+			}
+			memcpy(&ptr[qsize], buf, n);
+			qsize += n;
+			ptr[qsize] = 0;
+			*qstats = ptr;
+		}
+	}
+	return 1;
+}
+
+/* With the given cmd, query QMP for requested data. Returns allocated buffer containing data or NULL */
+static unsigned char *qmp_query(int qfd, char *cmd)
+{
+	unsigned char *qstats = NULL;
+	int n;
+
+	n = strlen(cmd);
+	if (qmp_write(qfd, cmd, n) != n)
+		return NULL;
+	if (!qmp_read(qfd, &qstats))
+		return NULL;
+	return qstats;
+}
+
+/* Returns a socket connected to the QMP socket. Returns -1 on failure. */
+static int qmp_connect(char *path)
+{
+	struct sockaddr_un sun;
+	int s;
+
+	if ((s = socket(AF_UNIX, SOCK_STREAM, 0)) < 0)
+		return -1;
+	(void)fcntl(s, F_SETFD, 1);
+
+	memset(&sun, 0, sizeof(struct sockaddr_un));
+	sun.sun_family = AF_UNIX;
+
+	if (strlen(path) >= sizeof(sun.sun_path)) {
+		close(s);
+		return -1;
+	}
+
+	strcpy(sun.sun_path, path);
+	if (connect(s, (struct sockaddr *)&sun, SUN_LEN(&sun)) < 0) {
+		close(s);
+		return -1;
+	}
+
+	return s;
+}
+
+/* Get up to 1024 active domains */
+static xc_domaininfo_t *get_domain_ids(int *num_doms)
+{
+	xc_domaininfo_t *dominfo;
+	xc_interface *xc_handle;
+
+	dominfo = calloc(1024, sizeof(xc_domaininfo_t));
+	if (dominfo == NULL)
+		return NULL;
+	xc_handle = xc_interface_open(0,0,0);
+	*num_doms = xc_domain_getinfolist(xc_handle, 0, 1024, dominfo);
+	xc_interface_close(xc_handle);
+	return dominfo;
+}
+
+/* Gather the qdisk statistics by querying QMP
+   Resources: http://wiki.qemu.org/QMP and qmp-commands.hx from the qemu code
+   QMP Syntax for entering command mode. This command must be issued before
+   issuing any other command:
+     In: {"execute": "qmp_capabilities"}
+     Out: {"return": {}}
+   QMP Syntax for querying block statistics:
+     In: { "execute": "query-blockstats" }
+     Out: {"return": [{
+            "device": 'str',
+            "parent": {
+              "stats": {
+                "flush_total_time_ns": 'int', "wr_highest_offset": 'int',
+                "wr_total_time_ns": 'int', "wr_bytes": 'int',
+                "rd_total_time_ns": 'int', "flush_operations": 'int',
+                "wr_operations": 'int', "rd_bytes": 'int', "rd_operations": 'int'
+              }
+            },
+            "stats": {
+              "flush_total_time_ns": 'int', "wr_highest_offset": 'int',
+              "wr_total_time_ns": 'int', "wr_bytes": 'int',
+              "rd_total_time_ns": 'int', "flush_operations": 'int',
+              "wr_operations": 'int', "rd_bytes": 'int', "rd_operations": 'int'
+            }
+          }]}
+*/
+void read_attributes_qdisk(xenstat_node * node)
+{
+	char *cmd_mode = "{ \"execute\": \"qmp_capabilities\" }";
+	char *query_blockstats_cmd = "{ \"execute\": \"query-blockstats\" }";
+	xc_domaininfo_t *dominfo = NULL;
+	unsigned char *qmp_stats, *val;
+	char path[80];
+	int i, qfd, num_doms;
+
+	dominfo = get_domain_ids(&num_doms);
+	if (dominfo == NULL)
+		return;
+
+	for (i=0; i<num_doms; i++) {
+		if (dominfo[i].domain <= 0)
+			continue;
+
+		/* Verify that qdisk disks are used with this VM */
+		snprintf(path, sizeof(path),"/local/domain/0/backend/qdisk/%i", dominfo[i].domain);
+		if ((val = xs_read(node->handle->xshandle, XBT_NULL, path, NULL)) == NULL)
+			continue;
+		free(val);
+
+		/* Connect to this VMs QMP socket */
+		snprintf(path, sizeof(path), "/var/run/xen/qmp-libxenstat-%i", dominfo[i].domain);
+		if ((qfd = qmp_connect(path)) < 0) {
+			continue;
+		}
+
+		/* First enable QMP capabilities so that we can query for data */
+		if ((qmp_stats = qmp_query(qfd, cmd_mode)) != NULL) {
+			free(qmp_stats);
+			/* Query QMP for this VMs blockstats */
+			if ((qmp_stats = qmp_query(qfd, query_blockstats_cmd)) != NULL) {
+				qmp_parse_stats(node, dominfo[i].domain, qmp_stats, qfd);
+				free(qmp_stats);
+			}
+		}
+		close(qfd);
+	}
+
+	free(dominfo);
+}
+
+#else /* !HAVE_YAJL_V2 */
+
+/* Statistics gathering for qdisks requires at least yajl v2 */
+void read_attributes_qdisk(xenstat_node * node)
+{
+}
+
+#endif /* !HAVE_YAJL_V2 */

Re: [PATCH v3] xentop: add support for qdisks

[Ian Campbell]

On Mon, 2015-03-23 at 20:55 -0600, Charles Arnold wrote:

> +/* Get up to 1024 active domains */

What I meant last time was, is this limitation a concern? What if there
are 1025 domains?

If this is a concern then perhaps refactor such that the qdisk stats
gathering collection can happen from the inner loop of xenstat_get_node,
i.e. near the call to domain_get_tmem_stats, then you would be given the
single domain of interest. Might simplify some other stuff too.

Ian.

Re: [PATCH v3] xentop: add support for qdisks

[Charles Arnold]

>>> On 3/24/2015 at 09:01 AM, Ian Campbell <ian.campbell@citrix.com> wrote: 
> On Mon, 2015-03-23 at 20:55 -0600, Charles Arnold wrote:
> 
>> +/* Get up to 1024 active domains */
> 
> What I meant last time was, is this limitation a concern? What if there
> are 1025 domains?

This is actually a limit I picked up from libxl.  See libxl.c libxl_list_domain()
which is consumed in several places.  I guess I assumed this was some official
limit.  This version of the patch would simply ignore any VMs beyond 1024.

> 
> If this is a concern then perhaps refactor such that the qdisk stats
> gathering collection can happen from the inner loop of xenstat_get_node,
> i.e. near the call to domain_get_tmem_stats, then you would be given the
> single domain of interest. Might simplify some other stuff too.

This would certainly eliminate the 1024 limit. This may become important
for running xentop in batch mode where the output can be captured. For
normal screen viewing, I doubt anyone has a screen with more that 1024
lines on which to view the output :)

I'll code up another version with your suggestion.

- Charles

Re: [PATCH v3] xentop: add support for qdisks

[Ian Campbell]

On Tue, 2015-03-24 at 10:24 -0600, Charles Arnold wrote:
> >>> On 3/24/2015 at 09:01 AM, Ian Campbell <ian.campbell@citrix.com> wrote: 
> > On Mon, 2015-03-23 at 20:55 -0600, Charles Arnold wrote:
> > 
> >> +/* Get up to 1024 active domains */
> > 
> > What I meant last time was, is this limitation a concern? What if there
> > are 1025 domains?
> 
> This is actually a limit I picked up from libxl.  See libxl.c libxl_list_domain()
> which is consumed in several places.  I guess I assumed this was some official
> limit.  This version of the patch would simply ignore any VMs beyond 1024.

Hrm, that libxl limit is also somewhat unfortunate :-(. In theory
xc_domain_getinfolist can be used a bit more flexibly to get slices of
domains, although getting a consistent snapshot might be tricky. One to
fix for sure though.

> > If this is a concern then perhaps refactor such that the qdisk stats
> > gathering collection can happen from the inner loop of xenstat_get_node,
> > i.e. near the call to domain_get_tmem_stats, then you would be given the
> > single domain of interest. Might simplify some other stuff too.
> 
> This would certainly eliminate the 1024 limit. This may become important
> for running xentop in batch mode where the output can be captured. For
> normal screen viewing, I doubt anyone has a screen with more that 1024
> lines on which to view the output :)

:-). I'm not sure if there is anyone out there who uses libxenstat
directly for other purposes, but I suppose it isn't impossible.

> I'll code up another version with your suggestion.

Thanks, given the libxl limit I'm wondering about just taking v3 of this
patch and taking what would otherwise have been v4 as an improvement.

Ian, Wei, any thoughts?

Ian.

Re: [PATCH v3] xentop: add support for qdisks

[Wei Liu]

On Tue, Mar 24, 2015 at 04:59:47PM +0000, Ian Campbell wrote:
[...]
> > for running xentop in batch mode where the output can be captured. For
> > normal screen viewing, I doubt anyone has a screen with more that 1024
> > lines on which to view the output :)
> 
> :-). I'm not sure if there is anyone out there who uses libxenstat
> directly for other purposes, but I suppose it isn't impossible.
> 
> > I'll code up another version with your suggestion.
> 
> Thanks, given the libxl limit I'm wondering about just taking v3 of this
> patch and taking what would otherwise have been v4 as an improvement.
> 

Agreed.

Wei.

Re: [PATCH v3] xentop: add support for qdisks

[Ian Campbell]

On Mon, 2015-03-30 at 16:20 +0100, Wei Liu wrote:
> On Tue, Mar 24, 2015 at 04:59:47PM +0000, Ian Campbell wrote:
> [...]
> > > for running xentop in batch mode where the output can be captured. For
> > > normal screen viewing, I doubt anyone has a screen with more that 1024
> > > lines on which to view the output :)
> > 
> > :-). I'm not sure if there is anyone out there who uses libxenstat
> > directly for other purposes, but I suppose it isn't impossible.
> > 
> > > I'll code up another version with your suggestion.
> > 
> > Thanks, given the libxl limit I'm wondering about just taking v3 of this
> > patch and taking what would otherwise have been v4 as an improvement.
> > 
> 
> Agreed.

OK, Acked + applied.

Ian.