[PATCH v3] xentop: Dynamically expand some columns

[PATCH v3] xentop: Dynamically expand some columns

[Charles Arnold]

Allow certain xentop columns to automatically expand as the amount
of data reported gets larger.  The columns allowed to auto expand are:

NETTX(k), NETRX(k), VBD_RD, VBD_WR, VBD_RSECT, VBD_WSECT

If the -f option is used to allow full length VM names, those names will
also be aligned based on the longest name in the NAME column.

The default minimum width of all columns remains unchanged.

Author: Markus Hauschild <Markus.Hauschild@rz.uni-regensburg.de>
Signed-off-by: Charles Arnold <carnold@suse.com>

diff --git a/tools/xenstat/xentop/Makefile b/tools/xenstat/xentop/Makefile
index 18bccb6..1797d84 100644
--- a/tools/xenstat/xentop/Makefile
+++ b/tools/xenstat/xentop/Makefile
@@ -18,13 +18,12 @@ ifneq ($(XENSTAT_XENTOP),y)
 all install xentop:
 else
 
-CFLAGS += -DGCC_PRINTF -Werror $(CFLAGS_libxenstat)
-LDLIBS += $(LDLIBS_libxenstat) $(CURSES_LIBS) $(SOCKET_LIBS)
+CFLAGS += -DGCC_PRINTF -Wall -Werror $(CFLAGS_libxenstat)
+LDLIBS += $(LDLIBS_libxenstat) $(CURSES_LIBS) $(SOCKET_LIBS) -lm
 CFLAGS += -DHOST_$(XEN_OS)
 
 # Include configure output (config.h) to headers search path
 CFLAGS += -I$(XEN_ROOT)/tools
-LDFLAGS += $(APPEND_LDFLAGS)
 
 .PHONY: all
 all: xentop
diff --git a/tools/xenstat/xentop/xentop.c b/tools/xenstat/xentop/xentop.c
index dd11927..df758dc 100644
--- a/tools/xenstat/xentop/xentop.c
+++ b/tools/xenstat/xentop/xentop.c
@@ -27,6 +27,7 @@
 
 #include <ctype.h>
 #include <errno.h>
+#include <math.h>
 #include <stdio.h>
 #include <stdlib.h>
 #include <stdarg.h>
@@ -127,7 +128,8 @@ static int compare_vbd_rsect(xenstat_domain *domain1, xenstat_domain *domain2);
 static void print_vbd_rsect(xenstat_domain *domain);
 static int compare_vbd_wsect(xenstat_domain *domain1, xenstat_domain *domain2);
 static void print_vbd_wsect(xenstat_domain *domain);
-
+static void reset_field_widths(void);
+static void adjust_field_widths(xenstat_domain *domain);
 
 /* Section printing functions */
 static void do_summary(void);
@@ -444,7 +446,7 @@ int compare_name(xenstat_domain *domain1, xenstat_domain *domain2)
 void print_name(xenstat_domain *domain)
 {
 	if(show_full_name)
-		print("%10s", xenstat_domain_name(domain));
+		print("%*s", fields[FIELD_NAME-1].default_width, xenstat_domain_name(domain));
 	else
 		print("%10.10s", xenstat_domain_name(domain));
 }
@@ -623,7 +625,7 @@ static int compare_net_tx(xenstat_domain *domain1, xenstat_domain *domain2)
 /* Prints number of total network tx bytes statistic */
 static void print_net_tx(xenstat_domain *domain)
 {
-	print("%8llu", tot_net_bytes(domain, FALSE)/1024);
+	print("%*llu", fields[FIELD_NET_TX-1].default_width, tot_net_bytes(domain, FALSE)/1024);
 }
 
 /* Compares number of total network rx bytes of two domains, returning -1,0,1
@@ -637,7 +639,7 @@ static int compare_net_rx(xenstat_domain *domain1, xenstat_domain *domain2)
 /* Prints number of total network rx bytes statistic */
 static void print_net_rx(xenstat_domain *domain)
 {
-	print("%8llu", tot_net_bytes(domain, TRUE)/1024);
+	print("%*llu", fields[FIELD_NET_RX-1].default_width, tot_net_bytes(domain, TRUE)/1024);
 }
 
 /* Gets number of total network bytes statistic, if rx true, then rx bytes
@@ -705,7 +707,7 @@ static int compare_vbd_rd(xenstat_domain *domain1, xenstat_domain *domain2)
 /* Prints number of total VBD READ requests statistic */
 static void print_vbd_rd(xenstat_domain *domain)
 {
-	print("%8llu", tot_vbd_reqs(domain, FIELD_VBD_RD));
+	print("%*llu", fields[FIELD_VBD_RD-1].default_width, tot_vbd_reqs(domain, FIELD_VBD_RD));
 }
 
 /* Compares number of total VBD WRITE requests of two domains,
@@ -719,7 +721,7 @@ static int compare_vbd_wr(xenstat_domain *domain1, xenstat_domain *domain2)
 /* Prints number of total VBD WRITE requests statistic */
 static void print_vbd_wr(xenstat_domain *domain)
 {
-	print("%8llu", tot_vbd_reqs(domain, FIELD_VBD_WR));
+	print("%*llu", fields[FIELD_VBD_WR-1].default_width, tot_vbd_reqs(domain, FIELD_VBD_WR));
 }
 
 /* Compares number of total VBD READ sectors of two domains,
@@ -733,7 +735,7 @@ static int compare_vbd_rsect(xenstat_domain *domain1, xenstat_domain *domain2)
 /* Prints number of total VBD READ sectors statistic */
 static void print_vbd_rsect(xenstat_domain *domain)
 {
-	print("%10llu", tot_vbd_reqs(domain, FIELD_VBD_RSECT));
+	print("%*llu", fields[FIELD_VBD_RSECT-1].default_width, tot_vbd_reqs(domain, FIELD_VBD_RSECT));
 }
 
 /* Compares number of total VBD WRITE sectors of two domains,
@@ -747,7 +749,7 @@ static int compare_vbd_wsect(xenstat_domain *domain1, xenstat_domain *domain2)
 /* Prints number of total VBD WRITE sectors statistic */
 static void print_vbd_wsect(xenstat_domain *domain)
 {
-	print("%10llu", tot_vbd_reqs(domain, FIELD_VBD_WSECT));
+	print("%*llu", fields[FIELD_VBD_WSECT-1].default_width, tot_vbd_reqs(domain, FIELD_VBD_WSECT));
 }
 
 
@@ -806,6 +808,54 @@ static void print_ssid(xenstat_domain *domain)
 	print("%4u", xenstat_domain_ssid(domain));
 }
 
+/* Resets default_width for fields with potentially large numbers */
+void reset_field_widths(void)
+{
+	fields[FIELD_NET_TX-1].default_width = 8;
+	fields[FIELD_NET_RX-1].default_width = 8;
+	fields[FIELD_VBD_RD-1].default_width = 8;
+	fields[FIELD_VBD_WR-1].default_width = 8;
+	fields[FIELD_VBD_RSECT-1].default_width = 10;
+	fields[FIELD_VBD_WSECT-1].default_width = 10;
+}
+
+/* Adjusts default_width for fields with potentially large numbers */
+void adjust_field_widths(xenstat_domain *domain)
+{
+	unsigned int length;
+
+	if (show_full_name) {
+		length = (unsigned int)strlen(xenstat_domain_name(domain));
+		if (length > fields[FIELD_NAME-1].default_width)
+			fields[FIELD_NAME-1].default_width = length;
+	}
+
+	length = (unsigned int)(log10(tot_net_bytes(domain, FALSE)/1024) + 1);
+	if (length > fields[FIELD_NET_TX-1].default_width)
+		fields[FIELD_NET_TX-1].default_width = length;
+
+	length = (unsigned int)(log10(tot_net_bytes(domain, TRUE)/1024) + 1);
+	if (length > fields[FIELD_NET_RX-1].default_width)
+		fields[FIELD_NET_RX-1].default_width = length;
+
+	length = (unsigned int)(log10(tot_vbd_reqs(domain, FIELD_VBD_RD)) + 1);
+	if (length > fields[FIELD_VBD_RD-1].default_width)
+		fields[FIELD_VBD_RD-1].default_width = length;
+
+	length = (unsigned int)(log10(tot_vbd_reqs(domain, FIELD_VBD_WR)) + 1);
+	if (length > fields[FIELD_VBD_WR-1].default_width)
+		fields[FIELD_VBD_WR-1].default_width = length;
+
+	length = (unsigned int)(log10(tot_vbd_reqs(domain, FIELD_VBD_RSECT)) + 1);
+	if (length > fields[FIELD_VBD_RSECT-1].default_width)
+		fields[FIELD_VBD_RSECT-1].default_width = length;
+
+	length = (unsigned int)(log10(tot_vbd_reqs(domain, FIELD_VBD_WSECT)) + 1);
+	if (length > fields[FIELD_VBD_WSECT-1].default_width)
+		fields[FIELD_VBD_WSECT-1].default_width = length;
+}
+
+
 /* Section printing functions */
 /* Prints the top summary, above the domain table */
 void do_summary(void)
@@ -1088,6 +1138,12 @@ static void top(void)
 	if(first_domain_index >= num_domains)
 		first_domain_index = num_domains-1;
 
+	/* Adjust default_width for fields with potentially large numbers */
+	reset_field_widths();
+	for (i = first_domain_index; i < num_domains; i++) {
+		adjust_field_widths(domains[i]);
+	}
+
 	for (i = first_domain_index; i < num_domains; i++) {
 		if(!batch && current_row() == lines()-1)
 			break;

Re: [PATCH v3] xentop: Dynamically expand some columns

[Ian Campbell]

On Thu, 2014-10-02 at 13:30 -0600, Charles Arnold wrote:
> Allow certain xentop columns to automatically expand as the amount
> of data reported gets larger.  The columns allowed to auto expand are:
> 
> NETTX(k), NETRX(k), VBD_RD, VBD_WR, VBD_RSECT, VBD_WSECT
> 
> If the -f option is used to allow full length VM names, those names will
> also be aligned based on the longest name in the NAME column.
> 
> The default minimum width of all columns remains unchanged.
> 
> Author: Markus Hauschild <Markus.Hauschild@rz.uni-regensburg.de>

If this is the case then the first line of the body of the mail should
be:

From: Markus Hauschild <Markus.Hauschild@rz.uni-regensburg.de>

Which causes git to record the correct thing.

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

I think we also need Markus' S-o-b, since he is the author.

http://wiki.xen.org/wiki/Submitting_Xen_Patches#Signing_off_a_patch
contains the statement of what you are asserting by providing your
S-o-b. Are you invoking clause (b) perhaps? Having a clear statement
from Markus would be preferable.

> diff --git a/tools/xenstat/xentop/Makefile b/tools/xenstat/xentop/Makefile
> index 18bccb6..1797d84 100644
> --- a/tools/xenstat/xentop/Makefile
> +++ b/tools/xenstat/xentop/Makefile
> @@ -18,13 +18,12 @@ ifneq ($(XENSTAT_XENTOP),y)
>  all install xentop:
>  else
>  
> -CFLAGS += -DGCC_PRINTF -Werror $(CFLAGS_libxenstat)
> -LDLIBS += $(LDLIBS_libxenstat) $(CURSES_LIBS) $(SOCKET_LIBS)
> +CFLAGS += -DGCC_PRINTF -Wall -Werror $(CFLAGS_libxenstat)
> +LDLIBS += $(LDLIBS_libxenstat) $(CURSES_LIBS) $(SOCKET_LIBS) -lm
>  CFLAGS += -DHOST_$(XEN_OS)
>  
>  # Include configure output (config.h) to headers search path
>  CFLAGS += -I$(XEN_ROOT)/tools
> -LDFLAGS += $(APPEND_LDFLAGS)

This and the addition of -Wall are not related to the patch and really
ought to be done separately if at all. The removal of APPEND_LDFLAGS in
particular is questionable. (adding Wall during a freeze is a bit too)

> +/* Resets default_width for fields with potentially large numbers */
> +void reset_field_widths(void)
> +{
> +	fields[FIELD_NET_TX-1].default_width = 8;

All of the -1's which this patch adds are a bit unfortunate (could be
avoided in some cases by passing the known field number down). As is the
rather adhoc nature of the handling of which fields to update.

I suppose we can live with it.

> +	fields[FIELD_NET_RX-1].default_width = 8;
> +	fields[FIELD_VBD_RD-1].default_width = 8;
> +	fields[FIELD_VBD_WR-1].default_width = 8;
> +	fields[FIELD_VBD_RSECT-1].default_width = 10;
> +	fields[FIELD_VBD_WSECT-1].default_width = 10;
> +}
> +
> +/* Adjusts default_width for fields with potentially large numbers */
> +void adjust_field_widths(xenstat_domain *domain)
> +{
> +	unsigned int length;
> +
> +	if (show_full_name) {
> +		length = (unsigned int)strlen(xenstat_domain_name(domain));

Is it really necessary to case the return value of strlen?

> +		if (length > fields[FIELD_NAME-1].default_width)
> +			fields[FIELD_NAME-1].default_width = length;
> +	}
> +
> +	length = (unsigned int)(log10(tot_net_bytes(domain, FALSE)/1024) + 1);

#define INT_FIELD_WIDTH(n) ((unsigned int)(log10(x) + 1))
used as
    length = INT_FIELD_WIDTH(tot_net_bytes(domain, FALSE)/1024)
perhaps? Confines the casting and rounding up to a single place.

Ian.

Re: [PATCH v3] xentop: Dynamically expand some columns

[Charles Arnold]

>>> On 10/20/2014 at 06:27 AM, Ian Campbell <Ian.Campbell@citrix.com> wrote: 
> On Thu, 2014-10-02 at 13:30 -0600, Charles Arnold wrote:
>> Allow certain xentop columns to automatically expand as the amount
>> of data reported gets larger.  The columns allowed to auto expand are:
>> 
>> NETTX(k), NETRX(k), VBD_RD, VBD_WR, VBD_RSECT, VBD_WSECT
>> 
>> If the -f option is used to allow full length VM names, those names will
>> also be aligned based on the longest name in the NAME column.
>> 
>> The default minimum width of all columns remains unchanged.
>> 
>> Author: Markus Hauschild <Markus.Hauschild@rz.uni-regensburg.de>
> 
> If this is the case then the first line of the body of the mail should
> be:
> 
> From: Markus Hauschild <Markus.Hauschild@rz.uni-regensburg.de>
> 
> Which causes git to record the correct thing.
> 
>> Signed-off-by: Charles Arnold <carnold@suse.com>
> 
> I think we also need Markus' S-o-b, since he is the author.
> 
> http://wiki.xen.org/wiki/Submitting_Xen_Patches#Signing_off_a_patch
> contains the statement of what you are asserting by providing your
> S-o-b. Are you invoking clause (b) perhaps? Having a clear statement
> from Markus would be preferable.

Ok. I'll ask him to sign off on the patch and submit it.


> 
>> diff --git a/tools/xenstat/xentop/Makefile b/tools/xenstat/xentop/Makefile
>> index 18bccb6..1797d84 100644
>> --- a/tools/xenstat/xentop/Makefile
>> +++ b/tools/xenstat/xentop/Makefile
>> @@ -18,13 +18,12 @@ ifneq ($(XENSTAT_XENTOP),y)
>>  all install xentop:
>>  else
>>  
>> -CFLAGS += -DGCC_PRINTF -Werror $(CFLAGS_libxenstat)
>> -LDLIBS += $(LDLIBS_libxenstat) $(CURSES_LIBS) $(SOCKET_LIBS)
>> +CFLAGS += -DGCC_PRINTF -Wall -Werror $(CFLAGS_libxenstat)
>> +LDLIBS += $(LDLIBS_libxenstat) $(CURSES_LIBS) $(SOCKET_LIBS) -lm
>>  CFLAGS += -DHOST_$(XEN_OS)
>>  
>>  # Include configure output (config.h) to headers search path
>>  CFLAGS += -I$(XEN_ROOT)/tools
>> -LDFLAGS += $(APPEND_LDFLAGS)
> 
> This and the addition of -Wall are not related to the patch and really
> ought to be done separately if at all. The removal of APPEND_LDFLAGS in
> particular is questionable. (adding Wall during a freeze is a bit too)
> 
>> +/* Resets default_width for fields with potentially large numbers */
>> +void reset_field_widths(void)
>> +{
>> +	fields[FIELD_NET_TX-1].default_width = 8;
> 
> All of the -1's which this patch adds are a bit unfortunate (could be
> avoided in some cases by passing the known field number down). As is the
> rather adhoc nature of the handling of which fields to update.
> 
> I suppose we can live with it.
> 
>> +	fields[FIELD_NET_RX-1].default_width = 8;
>> +	fields[FIELD_VBD_RD-1].default_width = 8;
>> +	fields[FIELD_VBD_WR-1].default_width = 8;
>> +	fields[FIELD_VBD_RSECT-1].default_width = 10;
>> +	fields[FIELD_VBD_WSECT-1].default_width = 10;
>> +}
>> +
>> +/* Adjusts default_width for fields with potentially large numbers */
>> +void adjust_field_widths(xenstat_domain *domain)
>> +{
>> +	unsigned int length;
>> +
>> +	if (show_full_name) {
>> +		length = (unsigned int)strlen(xenstat_domain_name(domain));
> 
> Is it really necessary to case the return value of strlen?

You are right.  It isn't necessary.  size_t is an unsigned int.

> 
>> +		if (length > fields[FIELD_NAME-1].default_width)
>> +			fields[FIELD_NAME-1].default_width = length;
>> +	}
>> +
>> +	length = (unsigned int)(log10(tot_net_bytes(domain, FALSE)/1024) + 1);
> 
> #define INT_FIELD_WIDTH(n) ((unsigned int)(log10(x) + 1))
> used as
>     length = INT_FIELD_WIDTH(tot_net_bytes(domain, FALSE)/1024)
> perhaps? Confines the casting and rounding up to a single place.

Good idea.  I'll make this change.

Thanks,

- Charles