[systemd-devel] [PATCH] systemd: Add systemd.setenv for /proc/cmdline parsing.

[Lennart Poettering]

On Mon, 06.02.12 16:21, Douglas, William (william.douglas at intel.com) wrote:

Heya,

your mailer applied line breaks to you patch. Please resend with
unbroken lines.

> +        } else if (startswith(word, "systemd.setenv=")) {
> +                char *eq;
> +                int r;
> +
> +                if (!(eq = strchr(word + 15, '=')))

For new code we try to avoid doing if checks and function calls in one
line. i.e. please use this:

    eq = strchr(word + 15, '=');
    if (!eq)

instead of

    if (!(eq = strchr(word + 15, '=')))

Yes, the older code still uses the latter syntax.

> +                        log_warning("Failed to parse setenv switch %s.
> Ignoring.", word + 15);

I think it would make sense to have this result in usetenv() instead of
a failure message.

> +                else {
> +                        *eq = 0;
> +                        r = setenv(word + 15, eq + 1, 1);
> +                        if (r)
> +                                log_warning("Failed setenv call with %s.
> Ignoring", strerror(-r));

This is incorrect. You need to use errno in strerror(), not -r.

> +                        *eq = '=';

Humpf. Not sure I like this. "word" is actually a const char, so you are
changing something const here. 

Note that the string passed here is actually sometimes from argv[],
which is visible outside of the local process through
/proc/1/cmdline. If for a short moment you make a change to it, and then
undo it, this will be visible outside. This will most likely not be a
problem, but is still quite ugly I'd claim.

Hence please, do an strdup() here, and free the string afterwards.

Lennart

-- 
Lennart Poettering - Red Hat, Inc.