From mboxrd@z Thu Jan 1 00:00:00 1970 From: Ian Campbell Subject: Re: [PATCH v3] xentop: Dynamically expand some columns Date: Mon, 20 Oct 2014 13:27:59 +0100 Message-ID: <1413808079.4087.11.camel@citrix.com> References: <542D535D02000091000DBEF4@prv-mh.provo.novell.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <542D535D02000091000DBEF4@prv-mh.provo.novell.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Charles Arnold Cc: xen-devel List-Id: xen-devel@lists.xenproject.org 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 If this is the case then the first line of the body of the mail should be: From: Markus Hauschild Which causes git to record the correct thing. > Signed-off-by: Charles Arnold 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.