<div dir="ltr">Hi Zbigniew,<div> </div><div>Thank you very much for reviewing. Please find my comments below. <div class="gmail_extra"> <div class="gmail_quote">On Mon, Apr 1, 2013 at 6:19 AM, Zbigniew Jędrzejewski-Szmek <<a href="mailto:zbyszek@in.waw.pl" target="_blank">zbyszek@in.waw.pl</a>> wrote: <blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="HOEnZb"><div class="h5">On Sun, Mar 31, 2013 at 08:22:05PM +0200, Umut Tezduyar wrote: > --- > src/cgtop/cgtop.c | 108 ++++++++++++++++++++++++++++++++++++----------------- > 1 files changed, 74 insertions(+), 34 deletions(-) > > diff --git a/src/cgtop/cgtop.c b/src/cgtop/cgtop.c > index eebebf0..0ef4fcc 100644 > --- a/src/cgtop/cgtop.c > +++ b/src/cgtop/cgtop.c > @@ -69,6 +69,11 @@ static enum { > ORDER_IO > } arg_order = ORDER_CPU; > > +static enum { > + CPU_PERCENT, > + CPU_TIME, > +} arg_cpu_type = CPU_PERCENT; > + > static void group_free(Group *g) { > assert(g); > > @@ -372,16 +377,22 @@ static int group_compare(const void*a, const void *b) { > return 1; > > if (arg_order == ORDER_CPU) { > - if (x->cpu_valid && y->cpu_valid) { > - > - if (x->cpu_fraction > y->cpu_fraction) > + if (arg_cpu_type == CPU_PERCENT) { > + if (x->cpu_valid && y->cpu_valid) { > + if (x->cpu_fraction > y->cpu_fraction) > + return -1; > + else if (x->cpu_fraction < y->cpu_fraction) > + return 1; > + } else if (x->cpu_valid) > return -1; > - else if (x->cpu_fraction < y->cpu_fraction) > + else if (y->cpu_valid) > return 1; > - } else if (x->cpu_valid) > - return -1; > - else if (y->cpu_valid) > - return 1; > + } else { > + if (x->cpu_usage > y->cpu_usage) > + return -1; > + else if (x->cpu_usage < y->cpu_usage) > + return 1; > + } > } > > if (arg_order == ORDER_TASKS) { > @@ -428,13 +439,16 @@ static int display(Hashmap *a) { > Iterator i; > Group *g; > Group **array; > - unsigned rows, path_columns, n = 0, j; > + signed path_columns; > + unsigned rows, n = 0, j, maxtcpu = 0, maxtpath = 0; > + char cpu_title[21]; > > assert(a); > > /* Set cursor to top left corner and clear screen */ > - fputs("\033[H" > - "\033[2J", stdout); > + if (on_tty()) > + fputs("\033[H" > + "\033[2J", stdout); > > array = alloca(sizeof(Group*) * hashmap_size(a)); > > @@ -444,33 +458,50 @@ static int display(Hashmap *a) { > > qsort(array, n, sizeof(Group*), group_compare); > > + /* Find longest names in one run */ > + for (j = 0; j < n; j++) { > + snprintf(cpu_title, sizeof(cpu_title), "%llu", array[j]->cpu_usage); > + if (strlen(cpu_title) > maxtcpu) > + maxtcpu = strlen(cpu_title); > + if (strlen(array[j]->path) > maxtpath) > + maxtpath = strlen(array[j]->path); </div></div>The compiler *might* be smart enough, but let's not depend on that. strlen() result should be cached. Maybe just use MAX? </blockquote><div style>Umut: Sounds reasonable. </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> <div><div class="h5"> > + } > + > + if (arg_cpu_type == CPU_PERCENT) > + snprintf(cpu_title, sizeof(cpu_title), "%6s", "%CPU"); > + else > + snprintf(cpu_title, sizeof(cpu_title), "%*s", maxtcpu, "CPU Time"); > + > rows = lines(); > if (rows <= 10) > rows = 10; > > - path_columns = columns() - 42; > - if (path_columns < 10) > - path_columns = 10; > - > - printf("%s%-*s%s %s%7s%s %s%6s%s %s%8s%s %s%8s%s %s%8s%s\n\n", > - arg_order == ORDER_PATH ? ANSI_HIGHLIGHT_ON : "", path_columns, "Path", > - arg_order == ORDER_PATH ? ANSI_HIGHLIGHT_OFF : "", > - arg_order == ORDER_TASKS ? ANSI_HIGHLIGHT_ON : "", "Tasks", > - arg_order == ORDER_TASKS ? ANSI_HIGHLIGHT_OFF : "", > - arg_order == ORDER_CPU ? ANSI_HIGHLIGHT_ON : "", "%CPU", > - arg_order == ORDER_CPU ? ANSI_HIGHLIGHT_OFF : "", > - arg_order == ORDER_MEMORY ? ANSI_HIGHLIGHT_ON : "", "Memory", > - arg_order == ORDER_MEMORY ? ANSI_HIGHLIGHT_OFF : "", > - arg_order == ORDER_IO ? ANSI_HIGHLIGHT_ON : "", "Input/s", > - arg_order == ORDER_IO ? ANSI_HIGHLIGHT_OFF : "", > - arg_order == ORDER_IO ? ANSI_HIGHLIGHT_ON : "", "Output/s", > - arg_order == ORDER_IO ? ANSI_HIGHLIGHT_OFF : ""); > + if (on_tty()) { > + path_columns = columns() - 36 - strlen(cpu_title); > + if (path_columns < 10) > + path_columns = 10; > + } else > + path_columns = maxtpath; > + > + printf("%s%-*s%s %s%7s%s %s%s%s %s%8s%s %s%8s%s %s%8s%s\n\n", > + on_tty() && arg_order == ORDER_PATH ? ANSI_HIGHLIGHT_ON : "", path_columns, "Path", > + on_tty() && arg_order == ORDER_PATH ? ANSI_HIGHLIGHT_OFF : "", > + on_tty() && arg_order == ORDER_TASKS ? ANSI_HIGHLIGHT_ON : "", "Tasks", > + on_tty() && arg_order == ORDER_TASKS ? ANSI_HIGHLIGHT_OFF : "", > + on_tty() && arg_order == ORDER_CPU ? ANSI_HIGHLIGHT_ON : "", cpu_title, > + on_tty() && arg_order == ORDER_CPU ? ANSI_HIGHLIGHT_OFF : "", > + on_tty() && arg_order == ORDER_MEMORY ? ANSI_HIGHLIGHT_ON : "", "Memory", > + on_tty() && arg_order == ORDER_MEMORY ? ANSI_HIGHLIGHT_OFF : "", > + on_tty() && arg_order == ORDER_IO ? ANSI_HIGHLIGHT_ON : "", "Input/s", > + on_tty() && arg_order == ORDER_IO ? ANSI_HIGHLIGHT_OFF : "", > + on_tty() && arg_order == ORDER_IO ? ANSI_HIGHLIGHT_ON : "", "Output/s", > + on_tty() && arg_order == ORDER_IO ? ANSI_HIGHLIGHT_OFF : ""); </div></div>What about defining const char *on = on_tty() ? ANSI_HIGHLIGHT_ON : "", *off = on_tty() ? ANSI_HIGHLIGHT_OFF : "", to make this easier to read? </blockquote><div style>Umut: I agree. </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> <div><div class="h5"> > > for (j = 0; j < n; j++) { > char *p; > char m[FORMAT_BYTES_MAX]; > > - if (j + 5 > rows) > + if (on_tty() && j + 5 > rows) > break; > > g = array[j]; > @@ -484,10 +515,13 @@ static int display(Hashmap *a) { > else > fputs(" -", stdout); > > - if (g->cpu_valid) > - printf(" %6.1f", g->cpu_fraction*100); > + if (arg_cpu_type == CPU_PERCENT) > + if (g->cpu_valid) > + printf(" %6.1f", g->cpu_fraction*100); > + else > + fputs(" -", stdout); > else > - fputs(" -", stdout); > + printf(" %*llu", maxtcpu, g->cpu_usage); > > if (g->memory_valid) > printf(" %8s", format_bytes(m, sizeof(m), g->memory)); > @@ -519,6 +553,7 @@ static void help(void) { > " -c Order by CPU load\n" > " -m Order by memory load\n" > " -i Order by IO load\n" > + " --cpu=TYPE time or percentage (default) as CPU type\n" > " -d --delay=DELAY Specify delay\n" > " -n --iterations=N Run for N iterations before exiting\n" > " -b --batch Run in batch mode, accepting no input\n" > @@ -535,6 +570,7 @@ static int parse_argv(int argc, char *argv[]) { > enum { > ARG_VERSION = 0x100, > ARG_DEPTH, > + ARG_CPU_TYPE > }; > > static const struct option options[] = { > @@ -544,6 +580,7 @@ static int parse_argv(int argc, char *argv[]) { > { "iterations", required_argument, NULL, 'n' }, > { "batch", no_argument, NULL, 'b' }, > { "depth", required_argument, NULL, ARG_DEPTH }, > + { "cpu", required_argument, NULL, ARG_CPU_TYPE}, </div></div>I think this should be optional_argument. </blockquote><div style>Umut: Didn't quite understand this. Please explain why you think it should be optional? </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> <div class="im"> > { NULL, 0, NULL, 0 } > }; > > @@ -565,6 +602,11 @@ static int parse_argv(int argc, char *argv[]) { > version(); > return 0; > > + case ARG_CPU_TYPE: > + if (strcmp(optarg, "time") == 0) > + arg_cpu_type = CPU_TIME; > + break; </div>This should check for errors, especially that we want cgtop to be scriptable. So an unsupported type should result in an error. </blockquote><div style>Umut: I will add the type limitation. </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> <div class="im"> > case ARG_DEPTH: > r = safe_atou(optarg, &arg_depth); > if (r < 0) { > @@ -777,8 +819,6 @@ int main(int argc, char *argv[]) { > } > } > > - log_info("Exiting."); > - > r = 0; </div>I thought that it was supposed to break off after one loop, if !on_tty(). It seems to loop. Also, for me is still prints too much headers: </blockquote><div style>Umut: I thought not to put a 1 loop limitation when not on tty. Someone might want to record data over a period of time. Thats why I kept the limit. I have also noticed the repeating column headers but the reason why I kept them was, to provide a splitter between consecutive data output. If the consensus is only printing them once, I will fix it.</div> <blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> $ systemd-cgtop|cat Path Tasks %CPU Memory Input/s Output/s / 395 18.8 2.7G - - ... I guess that you might be developing on 32 bits. Please put something like this on top: </blockquote><div style>Umut: I am indeed on 32 bit. I will use the format modifiers from stdint. </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> diff --git src/cgtop/cgtop.c src/cgtop/cgtop.c index 0ef4fcc..9f7ffd6 100644 --- src/cgtop/cgtop.c +++ src/cgtop/cgtop.c @@ -19,9 +19,11 @@ along with systemd; If not, see <<a href="http://www.gnu.org/licenses/" target="_blank">http://www.gnu.org/licenses/</a>>. ***/ +#define __STDC_FORMAT_MACROS #include <errno.h> #include <string.h> #include <stdlib.h> +#include <stdint.h> #include <unistd.h> #include <alloca.h> #include <getopt.h> @@ -460,7 +462,7 @@ static int display(Hashmap *a) { <div class="im"> /* Find longest names in one run */ </div><div class="im"> for (j = 0; j < n; j++) { - snprintf(cpu_title, sizeof(cpu_title), "%llu", array[j]->cpu_usage); </div>+ snprintf(cpu_title, sizeof(cpu_title), "%"PRIu64, array[j]->cpu_usage); if (strlen(cpu_title) > maxtcpu) maxtcpu = strlen(cpu_title); <div class="im"> if (strlen(array[j]->path) > maxtpath) </div>@@ -521,7 +523,7 @@ static int display(Hashmap *a) { <div class="im"> else fputs(" -", stdout); </div> else <div class="im">- printf(" %*llu", maxtcpu, g->cpu_usage); </div>+ printf(" %*"PRIu64, maxtcpu, g->cpu_usage); <div class="im"> if (g->memory_valid) printf(" %8s", format_bytes(m, sizeof(m), g->memory)); </div>HTH, Zbyszek </blockquote></div> </div></div></div>