[systemd-devel] [PATCH] cgtop: add options to format memory, IO usage in raw bytes

[Zbigniew Jędrzejewski-Szmek]

On Fri, May 22, 2015 at 05:44:47PM -0500, Charles Duffy wrote:
> From: Charles Duffy <chaduffy at cisco.com>
> 
> ---
>  src/cgtop/cgtop.c | 64 ++++++++++++++++++++++++++++++++++++++++++++-----------
>  1 file changed, 51 insertions(+), 13 deletions(-)
> 
> diff --git a/src/cgtop/cgtop.c b/src/cgtop/cgtop.c
> index a390cf3..45c8d6f 100644
> --- a/src/cgtop/cgtop.c
> +++ b/src/cgtop/cgtop.c
> @@ -77,6 +77,15 @@ static enum {
>          CPU_TIME,
>  } arg_cpu_type = CPU_PERCENT;
>  
> +enum ByteRepresentation {
> +        BYTES_HUMAN,
> +        BYTES_RAW,
> +};
> +typedef enum ByteRepresentation ByteRepresentation;
> +
> +static ByteRepresentation arg_memory_type = BYTES_HUMAN;
> +static ByteRepresentation arg_io_type = BYTES_HUMAN;
> +
>  static void group_free(Group *g) {
>          assert(g);
>  
> @@ -96,6 +105,16 @@ static void group_hashmap_free(Hashmap *h) {
>          hashmap_free(h);
>  }
>  
> +static char *cond_format_bytes(char *buf, size_t l, off_t t, ByteRepresentation r, bool is_valid) {
This should return const char *.

> +        if (!is_valid)
> +                return (char*)"       -";
Width formatting is already done in the caller of this function.
So just "-" should be enough.

> +        if (r == BYTES_HUMAN)
> +                return format_bytes(buf, l, t);
> +        snprintf(buf, l, "%lu8", t);
What is "8" doing in the format? Also off_t is signed. I think "%zd" is
the right format.

> +        buf[l-1] = 0;
snprintf terminates with NUL on its own, always, so this line is not needed.

Zbyszek

> +        return buf;
> +}
> +
>  static int process(const char *controller, const char *path, Hashmap *a, Hashmap *b, unsigned iteration) {
>          Group *g;
>          int r;
> @@ -532,18 +551,9 @@ static int display(Hashmap *a) {
>                  } else
>                          printf(" %*s", maxtcpu, format_timespan(buffer, sizeof(buffer), (nsec_t) (g->cpu_usage / NSEC_PER_USEC), 0));
>  
> -                if (g->memory_valid)
> -                        printf(" %8s", format_bytes(buffer, sizeof(buffer), g->memory));
> -                else
> -                        fputs("        -", stdout);
> -
> -                if (g->io_valid) {
> -                        printf(" %8s",
> -                               format_bytes(buffer, sizeof(buffer), g->io_input_bps));
> -                        printf(" %8s",
> -                               format_bytes(buffer, sizeof(buffer), g->io_output_bps));
> -                } else
> -                        fputs("        -        -", stdout);
> +                printf(" %8s", cond_format_bytes(buffer, sizeof(buffer), g->memory, arg_memory_type, g->memory_valid));
> +                printf(" %8s", cond_format_bytes(buffer, sizeof(buffer), g->io_input_bps, arg_io_type, g->io_valid));
> +                printf(" %8s", cond_format_bytes(buffer, sizeof(buffer), g->io_output_bps, arg_io_type, g->io_valid));
>  
>                  putchar('\n');
>          }
> @@ -562,6 +572,8 @@ static void help(void) {
>                 "  -m                  Order by memory load\n"
>                 "  -i                  Order by IO load\n"
>                 "     --cpu[=TYPE]     Show CPU usage as time or percentage (default)\n"
> +               "     --memory[=TYPE]  Show memory usage as bytes or human (default)\n"
> +               "     --io[=TYPE]      Show memory usage as bytes or human (default)\n"
So, I know that Umut suggested this, but I'm not sure that this is a good
idea. When would you want different units for memory and io? One switch
should be enough.

Zbyszek

>                 "  -d --delay=DELAY    Delay between updates\n"
>                 "  -n --iterations=N   Run for N iterations before exiting\n"
>                 "  -b --batch          Run in batch mode, accepting no input\n"
> @@ -574,7 +586,9 @@ static int parse_argv(int argc, char *argv[]) {
>          enum {
>                  ARG_VERSION = 0x100,
>                  ARG_DEPTH,
> -                ARG_CPU_TYPE
> +                ARG_CPU_TYPE,
> +                ARG_MEM_TYPE,
> +                ARG_IO_TYPE,
>          };
>  
>          static const struct option options[] = {
> @@ -585,6 +599,8 @@ static int parse_argv(int argc, char *argv[]) {
>                  { "batch",      no_argument,       NULL, 'b'         },
>                  { "depth",      required_argument, NULL, ARG_DEPTH   },
>                  { "cpu",        optional_argument, NULL, ARG_CPU_TYPE},
> +                { "memory",     optional_argument, NULL, ARG_MEM_TYPE},
> +                { "io",         optional_argument, NULL, ARG_IO_TYPE },
>                  {}
>          };
>  
> @@ -618,6 +634,28 @@ static int parse_argv(int argc, char *argv[]) {
>                          }
>                          break;
>  
> +                case ARG_MEM_TYPE:
> +                        if (optarg) {
> +                                if (strcmp(optarg, "human") == 0)
> +                                        arg_memory_type = BYTES_HUMAN;
> +                                else if (strcmp(optarg, "bytes") == 0)
> +                                        arg_memory_type = BYTES_RAW;
> +                                else
> +                                        return -EINVAL;
> +                        }
> +                        break;
> +
> +                case ARG_IO_TYPE:
> +                        if (optarg) {
> +                                if (strcmp(optarg, "human") == 0)
> +                                        arg_io_type = BYTES_HUMAN;
> +                                else if (strcmp(optarg, "bytes") == 0)
> +                                        arg_io_type = BYTES_RAW;
> +                                else
> +                                        return -EINVAL;
> +                        }
> +                        break;
> +
>                  case ARG_DEPTH:
>                          r = safe_atou(optarg, &arg_depth);
>                          if (r < 0) {
> -- 
> 2.0.0
> 
> _______________________________________________
> systemd-devel mailing list
> systemd-devel at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/systemd-devel