[PATCH v2 0/4] libxenstat bug fixes and cleanups

[PATCH v2 0/4] libxenstat bug fixes and cleanups

[Wei Liu]

Wei Liu (4):
  libxenstat: reuse xc_handle open in xenstat_init
  libxenstat: YAJL_GET_STRING may return NULL
  libxenstat: always free qmp_stats
  libxenstat: qmp_read fix and cleanup

 tools/xenstat/libxenstat/src/xenstat_qmp.c | 19 ++++++++-----------
 1 file changed, 8 insertions(+), 11 deletions(-)

-- 
1.9.1

[PATCH v2 1/4] libxenstat: reuse xc_handle open in xenstat_init

[Wei Liu]

Signed-off-by: Wei Liu <wei.liu2@citrix.com>
Cc: Ian Campbell <ian.campbell@citrix.com>
Cc: Ian Jackson <ian.jackson@eu.citrix.com>
Cc: Charles Arnold <carnold@suse.com>
---
 tools/xenstat/libxenstat/src/xenstat_qmp.c | 7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)

diff --git a/tools/xenstat/libxenstat/src/xenstat_qmp.c b/tools/xenstat/libxenstat/src/xenstat_qmp.c
index 2cb99e9..10ae104 100644
--- a/tools/xenstat/libxenstat/src/xenstat_qmp.c
+++ b/tools/xenstat/libxenstat/src/xenstat_qmp.c
@@ -357,17 +357,14 @@ static int qmp_connect(char *path)
 }
 
 /* Get up to 1024 active domains */
-static xc_domaininfo_t *get_domain_ids(int *num_doms)
+static xc_domaininfo_t *get_domain_ids(xc_interface *xc_handle, 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;
 }
 
@@ -406,7 +403,7 @@ void read_attributes_qdisk(xenstat_node * node)
 	char path[80];
 	int i, qfd, num_doms;
 
-	dominfo = get_domain_ids(&num_doms);
+	dominfo = get_domain_ids(node->handle->xc_handle, &num_doms);
 	if (dominfo == NULL)
 		return;
 
-- 
1.9.1

[PATCH v2 2/4] libxenstat: YAJL_GET_STRING may return NULL

[Wei Liu]

Passing NULL to strcmp can cause segmentation fault. Continue in that
case.

Signed-off-by: Wei Liu <wei.liu2@citrix.com>
Cc: Ian Campbell <ian.campbell@citrix.com>
Cc: Ian Jackson <ian.jackson@eu.citrix.com>
Cc: Charles Arnold <carnold@suse.com>
Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>
Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>
---
 tools/xenstat/libxenstat/src/xenstat_qmp.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/xenstat/libxenstat/src/xenstat_qmp.c b/tools/xenstat/libxenstat/src/xenstat_qmp.c
index 10ae104..aad15c8 100644
--- a/tools/xenstat/libxenstat/src/xenstat_qmp.c
+++ b/tools/xenstat/libxenstat/src/xenstat_qmp.c
@@ -110,7 +110,7 @@ static char *qmp_get_block_image(xenstat_node *node, char *qmp_devname, int qfd)
 		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))
+			if (!tmp || strcmp(qmp_devname, tmp))
 				continue;
 		}
 		else
-- 
1.9.1

[PATCH v2 3/4] libxenstat: always free qmp_stats

[Wei Liu]

Originally qmp_stats is only freed in failure path and leaked in success
path.

Instead of wiring up the success path, rearrange the code a bit to
always free qmp_stats before checking if info is NULL.

Signed-off-by: Wei Liu <wei.liu2@citrix.com>
Cc: Ian Campbell <ian.campbell@citrix.com>
Cc: Ian Jackson <ian.jackson@eu.citrix.com>
Cc: Charles Arnold <carnold@suse.com>
Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>
---
 tools/xenstat/libxenstat/src/xenstat_qmp.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/tools/xenstat/libxenstat/src/xenstat_qmp.c b/tools/xenstat/libxenstat/src/xenstat_qmp.c
index aad15c8..8d91fef 100644
--- a/tools/xenstat/libxenstat/src/xenstat_qmp.c
+++ b/tools/xenstat/libxenstat/src/xenstat_qmp.c
@@ -95,10 +95,10 @@ static char *qmp_get_block_image(xenstat_node *node, char *qmp_devname, int qfd)
 		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);
+	info = yajl_tree_parse((char *)qmp_stats, NULL, 0);
+	free(qmp_stats);
+	if (info == NULL)
 		return NULL;
-	}
 
 	ptr[0] = qblock[QMP_BLOCK_RETURN]; /* "return" */
 	if ((ret_obj = yajl_tree_get(info, ptr, yajl_t_array)) == NULL)
-- 
1.9.1

[PATCH v2 4/4] libxenstat: qmp_read fix and cleanup

[Wei Liu]

The second argument of poll(2) is the number of file descriptors. POLLIN
is defined as 1 so it happens to work. Also reduce the size of array to
one as there is only one file descriptor.

Signed-off-by: Wei Liu <wei.liu2@citrix.com>
Cc: Ian Campbell <ian.campbell@citrix.com>
Cc: Ian Jackson <ian.jackson@eu.citrix.com>
Cc: Charles Arnold <carnold@suse.com>
---
 tools/xenstat/libxenstat/src/xenstat_qmp.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/tools/xenstat/libxenstat/src/xenstat_qmp.c b/tools/xenstat/libxenstat/src/xenstat_qmp.c
index 8d91fef..5e261af 100644
--- a/tools/xenstat/libxenstat/src/xenstat_qmp.c
+++ b/tools/xenstat/libxenstat/src/xenstat_qmp.c
@@ -289,13 +289,13 @@ static size_t qmp_write(int qfd, char *cmd, size_t cmd_len)
 static int qmp_read(int qfd, unsigned char **qstats)
 {
 	unsigned char buf[1024], *ptr;
-	struct pollfd pfd[2];
+	struct pollfd pfd[1];
 	int n, qsize = 0;
 
 	*qstats = NULL;
 	pfd[0].fd = qfd;
 	pfd[0].events = POLLIN;
-	while ((n = poll(pfd, POLLIN, 10)) > 0) {
+	while ((n = poll(pfd, 1, 10)) > 0) {
 		if (pfd[0].revents & POLLIN) {
 			if ((n = read(qfd, buf, sizeof(buf))) < 0) {
 				free(*qstats);
-- 
1.9.1

Re: [PATCH v2 1/4] libxenstat: reuse xc_handle open in xenstat_init

[Andrew Cooper]

On 08/04/15 17:08, Wei Liu wrote:
> Signed-off-by: Wei Liu <wei.liu2@citrix.com>
> Cc: Ian Campbell <ian.campbell@citrix.com>
> Cc: Ian Jackson <ian.jackson@eu.citrix.com>
> Cc: Charles Arnold <carnold@suse.com>

Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>

> ---
>  tools/xenstat/libxenstat/src/xenstat_qmp.c | 7 ++-----
>  1 file changed, 2 insertions(+), 5 deletions(-)
>
> diff --git a/tools/xenstat/libxenstat/src/xenstat_qmp.c b/tools/xenstat/libxenstat/src/xenstat_qmp.c
> index 2cb99e9..10ae104 100644
> --- a/tools/xenstat/libxenstat/src/xenstat_qmp.c
> +++ b/tools/xenstat/libxenstat/src/xenstat_qmp.c
> @@ -357,17 +357,14 @@ static int qmp_connect(char *path)
>  }
>  
>  /* Get up to 1024 active domains */
> -static xc_domaininfo_t *get_domain_ids(int *num_doms)
> +static xc_domaininfo_t *get_domain_ids(xc_interface *xc_handle, 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;
>  }
>  
> @@ -406,7 +403,7 @@ void read_attributes_qdisk(xenstat_node * node)
>  	char path[80];
>  	int i, qfd, num_doms;
>  
> -	dominfo = get_domain_ids(&num_doms);
> +	dominfo = get_domain_ids(node->handle->xc_handle, &num_doms);
>  	if (dominfo == NULL)
>  		return;
>  

Re: [PATCH v2 4/4] libxenstat: qmp_read fix and cleanup

[Andrew Cooper]

On 08/04/15 17:08, Wei Liu wrote:
> The second argument of poll(2) is the number of file descriptors. POLLIN
> is defined as 1 so it happens to work. Also reduce the size of array to
> one as there is only one file descriptor.
>
> Signed-off-by: Wei Liu <wei.liu2@citrix.com>
> Cc: Ian Campbell <ian.campbell@citrix.com>
> Cc: Ian Jackson <ian.jackson@eu.citrix.com>
> Cc: Charles Arnold <carnold@suse.com>

Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>

> ---
>  tools/xenstat/libxenstat/src/xenstat_qmp.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/tools/xenstat/libxenstat/src/xenstat_qmp.c b/tools/xenstat/libxenstat/src/xenstat_qmp.c
> index 8d91fef..5e261af 100644
> --- a/tools/xenstat/libxenstat/src/xenstat_qmp.c
> +++ b/tools/xenstat/libxenstat/src/xenstat_qmp.c
> @@ -289,13 +289,13 @@ static size_t qmp_write(int qfd, char *cmd, size_t cmd_len)
>  static int qmp_read(int qfd, unsigned char **qstats)
>  {
>  	unsigned char buf[1024], *ptr;
> -	struct pollfd pfd[2];
> +	struct pollfd pfd[1];
>  	int n, qsize = 0;
>  
>  	*qstats = NULL;
>  	pfd[0].fd = qfd;
>  	pfd[0].events = POLLIN;
> -	while ((n = poll(pfd, POLLIN, 10)) > 0) {
> +	while ((n = poll(pfd, 1, 10)) > 0) {
>  		if (pfd[0].revents & POLLIN) {
>  			if ((n = read(qfd, buf, sizeof(buf))) < 0) {
>  				free(*qstats);

Re: [PATCH v2 0/4] libxenstat bug fixes and cleanups

[Ian Campbell]

On Wed, 2015-04-08 at 17:08 +0100, Wei Liu wrote:
> Wei Liu (4):
>   libxenstat: reuse xc_handle open in xenstat_init
>   libxenstat: YAJL_GET_STRING may return NULL
>   libxenstat: always free qmp_stats
>   libxenstat: qmp_read fix and cleanup
> 
>  tools/xenstat/libxenstat/src/xenstat_qmp.c | 19 ++++++++-----------
>  1 file changed, 8 insertions(+), 11 deletions(-)

All acked (FWIW given I supposedly reviewed the initial code, thanks for
fixing!) and applied, thanks.