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

[Ian Campbell <Ian.Campbell@citrix.com>]

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.