Hi, I have resubmitted the patch with agreed changes.
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. Thanks. On Mon, Apr 1, 2013 at 2:23 PM, Zbigniew Jędrzejewski-Szmek < [email protected]> wrote: > 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 < > > [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? > Because --cpu=percentange is the default. So the part after = (arg), > 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. > 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? > > > > $ 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), > > Zbyszek >
_______________________________________________ systemd-devel mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/systemd-devel
