Re: [Xen-devel] [PATCH v3] xentop: Dynamically expand some columns

["Charles Arnold" <carnold@xxxxxxxx>]

>>> On 10/20/2014 at 06:27 AM, Ian Campbell <Ian.Campbell@xxxxxxxxxx> 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@xxxxxxxxxxxxxxxxxxxx>
> 
> If this is the case then the first line of the body of the mail should
> be:
> 
> From: Markus Hauschild <Markus.Hauschild@xxxxxxxxxxxxxxxxxxxx>
> 
> Which causes git to record the correct thing.
> 
>> Signed-off-by: Charles Arnold <carnold@xxxxxxxx>
> 
> 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


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel