[systemd-devel] [PATCH 3/4] util: add function getting proc environ

[Lennart Poettering]

On Wed, 19.11.14 11:01, Jakub Filak (jfilak at redhat.com) wrote:

>  
> +int get_process_environ(pid_t pid, char **environ) {

If this is really just about pushing this into the journal: the
journal is actually binary safe, we could just drop the data there
without escaping it. That said, it certainly doesn't help readability,
and processability if fields are unnecesarily binary, hence it's
probably a good idea to escape things as your patch does it.

> +        _cleanup_fclose_ FILE *f = NULL;
> +        _cleanup_free_ char *outcome = NULL;
> +        int c;
> +        const char *p;
> +        char escaped[4];
> +        size_t allocated = 0, sz = 0, escaped_len = 0;
> +
> +        assert(pid >= 0);
> +        assert(environ);
> +
> +        p = procfs_file_alloca(pid, "environ");
> +
> +        f = fopen(p, "re");
> +        if (!f)
> +                return -errno;
> +
> +        while ((c = fgetc(f)) != EOF) {
> +                if (c == '\0') {
> +                        escaped[0] = '\n';
> +                        escaped_len = 1;
> +                }
> +                else
> +                        escaped_len = cescape_char(c, escaped);
> +
> +                if (!GREEDY_REALLOC(outcome, allocated, sz + escaped_len + 1))
> +                        return -ENOMEM;
> +
> +                memcpy(outcome + sz, escaped, escaped_len);
> +                sz += escaped_len;
> +        }

I'd probably just always prealloc 5 chars more in each iteration and
then use cescape_char() directly on the resulting buffer instead of
copying things into a temporary buffer first, and then reallocating to
the precise length and then copying it over to the real buffer.

It not only makes the code a bit quicker but also is most likely a bit
wiser resource-wise since the extra copy and realloc is usually more
expensive than a few bytes of memory added extra.

Otherwise looks great!

Lennart

-- 
Lennart Poettering, Red Hat