[systemd-devel] [PATCH] do not ellipsize cgroup members in full status

[Lennart Poettering]

On Tue, 15.01.13 10:58, Lukáš Nykrýn (lnykryn at redhat.com) wrote:

Heya,

> diff --git a/src/shared/output.h b/src/shared/output.h
> new file mode 100644
> index 0000000..0efd430
> --- /dev/null
> +++ b/src/shared/output.h

"output.h" is a bit too generic for my test, maybe a better name would
be output-mode.h or so?

>          if (pid == 0)
> @@ -919,7 +916,7 @@ int get_process_cmdline(pid_t pid, size_t max_length, bool comm_fallback, char *
>          else {
>                  char *p;
>                  if (asprintf(&p, "/proc/%lu/cmdline", (unsigned long) pid) < 0)
> -                        return -ENOMEM;
> +                        return log_oom();

Library calls shouldn't log on their own, except on debug level. Only
app code should. The line between "library" and "app" in systemd is a
blurry one as we have everything in the same repo, and link things
together in varying combinations, but util.c is definitely of the
"library" category and should hence not log errors directly, not even
for OOM, and just return ENOMEM;

>  
>                  f = fopen(p, "re");
>                  free(p);
> @@ -927,47 +924,66 @@ int get_process_cmdline(pid_t pid, size_t max_length, bool comm_fallback, char *
>  
>          if (!f)
>                  return -errno;
> +        if (max_length == 0) {
> +                size_t len = 0, alloc = 0;
> +                while ((c = getc(f)) != EOF) {
> +                        if(alloc <= len+1) {
> +                                alloc += 20;
> +                                k = realloc(r, alloc);

As I learnt recently realloc() is actually kinda smart internally, and
will increase the allocated buffer in chunks anyway, so that jumping 20
bytes at a time here (or even doing exponential increases as a lot of
code) is actually pretty much unnecessary, and we could just call
realloc() here always with the byte-exact length we need. Doesn't really
make anything slower or faster, but certainly makes thing simpler to read...

> +                                if (k == NULL) {
> +                                        free(r);
> +                                        fclose(f);
> +                                        return log_oom();

No log_oom() here either, please

> +                                }
> +                                r = k;
> +                        }
> +                        r[len] = isprint(c) ? c : ' ';
> +                        r[++len] = 0;
> +                }
> +        } else {
> +                bool space = false;
> +                size_t left;
> +                r = new(char, max_length);
> +                if (!r) {
> +                        fclose(f);
> +                        return log_oom();
> +                }

Otherwise looks good!

Lennart

-- 
Lennart Poettering - Red Hat, Inc.