On Mon, 06.02.12 16:21, Douglas, William ([email protected]) 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.
_______________________________________________
systemd-devel mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/systemd-devel
