Hi Zbigniew, Thank you very much for reviewing. Please find my comments below.
On Mon, Apr 1, 2013 at 6:19 AM, Zbigniew Jędrzejewski-Szmek < [email protected]> wrote: > 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); > The compiler *might* be smart enough, but let's not depend on that. > strlen() result should be cached. Maybe just use MAX? > Umut: Sounds reasonable. > > > + } > > + > > + 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 : ""); > What about defining > const char *on = on_tty() ? ANSI_HIGHLIGHT_ON : "", *off = on_tty() ? > ANSI_HIGHLIGHT_OFF : "", > to make this easier to read? > Umut: I agree. > > > > > 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}, > I think this should be optional_argument. > Umut: Didn't quite understand this. Please explain why you think it should be optional? > > > { 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; > This should check for errors, especially that we want cgtop to be > scriptable. So an unsupported type should result in an error. > Umut: I will add the type limitation. > > > 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; > 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: > 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. > > $ 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: > Umut: I am indeed on 32 bit. I will use the format modifiers from stdint. > > 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 <http://www.gnu.org/licenses/>. > ***/ > > +#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) { > > /* Find longest names in one run */ > for (j = 0; j < n; j++) { > - snprintf(cpu_title, sizeof(cpu_title), "%llu", > array[j]->cpu_usage); > + snprintf(cpu_title, sizeof(cpu_title), "%"PRIu64, > array[j]->cpu_usage); > if (strlen(cpu_title) > maxtcpu) > maxtcpu = strlen(cpu_title); > if (strlen(array[j]->path) > maxtpath) > @@ -521,7 +523,7 @@ static int display(Hashmap *a) { > else > fputs(" -", stdout); > else > - printf(" %*llu", maxtcpu, g->cpu_usage); > + printf(" %*"PRIu64, maxtcpu, g->cpu_usage); > > if (g->memory_valid) > printf(" %8s", format_bytes(m, sizeof(m), > g->memory)); > > HTH, > Zbyszek >
_______________________________________________ systemd-devel mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/systemd-devel
