[systemd-devel] [PATCH 2/2] Adding binary to shutdown the system

[Lennart Poettering]

On Wed, 06.10.10 02:05, Gustavo Sverzut Barbieri (barbieri at profusion.mobi) wrote:

> +static int send_signal(int sign) {
> +        sigset_t mask, oldmask;
> +        usec_t until;
> +        int processes;
> +        struct timespec ts;
> +
> +        sigemptyset(&mask);

Minor nitpick: I tend to enclose invocations like this with assert_se(x
== 0), e.g.

assert_se(sigemptyset(&mask) == 0);

This makes clear for the reader that there is is no realistic way this
could ever fail. But then again, this doesn't really matter.

> +        for (;;) {
> +                usec_t n;
> +
> +                for (;;) {
> +                        pid_t pid;
> +
> +                        n = now(CLOCK_MONOTONIC);
> +                        if (n >= until)
> +                                goto finish;

Put these two lines in the outer loop. There should be no need to query
the time each time.

> +static int rescue_send_signal(int sign) {
> +        sigset_t mask, oldmask;
> +        usec_t until;
> +        struct timespec ts;
> +        int r;
> +

Hmm, this  function does mostly the  same as send_signal()  but lets the
kernel iterate,  right? Given how similar  the functions I  wonder if it
would be nicer to use a bool argument to switch between the two modes...

> +int main(int argc, char *argv[]) {
> +        int cmd, r, retries;
> +        bool need_umount = true, need_swapoff = true, need_loop_detach = true;
> +
> +        log_parse_environment();
> +        log_set_target(LOG_TARGET_KMSG); /* syslog will die if not gone yet */
> +        log_open();

Hmm, if logging does not work for you, maybe you disabled kernel console
logging for debug messages? An option might be to raise the kernel log
level here?

> +
> +        if (getpid() != 1) {
> +                log_debug("only init may exec this binary");

I think this should be log_error(). Also, please use proper sentences,
i.e. uppercase the first char and add a full stop to the end.

> +                r = -EPERM;
> +                goto error;
> +        }
> +
> +        if (argc != 2) {
> +                log_debug("invalid number of arguments");

Siimilar here.

> +                r = -EINVAL;
> +                goto error;
> +        }
> +
> +        if (streq(argv[1], "reboot"))
> +                cmd = RB_AUTOBOOT;
> +        else if (streq(argv[1], "poweroff"))
> +                cmd = RB_POWER_OFF;
> +        else if (streq(argv[1], "halt"))
> +                cmd = RB_HALT_SYSTEM;
> +        else if (streq(argv[1], "kexec"))
> +                cmd = LINUX_REBOOT_CMD_KEXEC;
> +        else {
> +                log_debug("unknown action %s", argv[1]);
> +                r = -EINVAL;

And here.

> +        if (cmd == LINUX_REBOOT_CMD_KEXEC) {
> +                const char *args[5] = {KEXEC_BINARY_PATH, "-e", "-x", "-f", NULL};
> +                /* we cheat and exec kexec to avoid doing all its work */
> +                execv(args[0], (char * const *) args);
> +                log_warning("kexec not supported, falling back to reboot");
> +                cmd = RB_AUTOBOOT;
> +        }

Hmm, I wonder if we might need to fork things here... If we justz exec
kexec and it fails for some reason, is it capabale of invoking the
fallback reboot(RB_AUTOBOOT) itself? If it isn't we should fork off
kexec here, and wait for it, and if it fails do our own
reboot(RB_AUTOBOOT) instead.

Otherwise looks fantastic.

Lennart

-- 
Lennart Poettering - Red Hat, Inc.