<div dir="ltr">Hi,<div> </div><div style>I have resubmitted the patch with agreed changes. </div><div style> </div><div style>My usecase is pretty simple actually. I would like to store process time of services right after default.target and store the values in a database. That involves running cgtop 1 time and parsing the output. </div> <div style> </div><div style>Thanks.</div></div><div class="gmail_extra"> <div class="gmail_quote">On Mon, Apr 1, 2013 at 2:23 PM, 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 Mon, Apr 01, 2013 at 10:01:40AM +0200, Umut Tezduyar wrote: > 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 < > <a href="mailto:zbyszek@in.waw.pl">zbyszek@in.waw.pl</a>> 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? </div></div>Because --cpu=percentange is the default. So the part after = (arg), should be optional. <div><div class="h5"> > > > { 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. </div></div>Oh, OK. Would have nice to have this in the message description. The headers are probably not useful, if anything the empty line between consecutive outputs is good enough. I'm not convinced about the loop, though. It would not be very easy to use in scripting, because one would have to split the output. Actually invoking cgtop multiple times. Do we actually have a concrete example how this is to be used? <div><div class="h5"> > > $ 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 <<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) { > > > > /* 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), </div></div>Zbyszek </blockquote></div> </div>