[systemd-devel] [PATCH 3/4] Adding halt binary to shutdown the system

[Lennart Poettering]

On Tue, 05.10.10 08:41, fidencio at profusion.mobi (fidencio at profusion.mobi) wrote:

> +
> +#define TIMEOUT_USEC    5000000

To make this more readable please write 5*USEC_PER_SEC here.

> +static bool ignore_proc(pid_t pid) {
> +        if (pid == 1)
> +                return true;
> +
> +        return false;
> +}
> +
> +static unsigned int killall(DIR *dir, int sign) {
> +        struct dirent *d;
> +        pid_t pid;
> +        unsigned int processes = 0;
> +
> +        while ((d = readdir(dir))) {
> +
> +                if ((pid = atoi(d->d_name)) == 0)
> +                        continue;

atoi() is evil... please use parse_pid() here!

> +
> +                if (ignore_proc(pid))
> +                        continue;
> +
> +                kill(pid, sign);
> +                processes++;

We should increase the counter here only if the kill actually worked I believe.

> +        }
> +
> +        return processes;
> +}
> +
> +static unsigned int send_signal(DIR *dir, int sign) {
> +        sigset_t mask;
> +        usec_t until;
> +        unsigned int processes;
> +        struct timespec ts;
> +
> +        sigemptyset(&mask);
> +        sigaddset(&mask, SIGCHLD);

sigprocmask() is missing here? You need to make sure SIGCHLD is not
delivered via signal handlers here anymore.

> +        processes = killall(dir, sign);
> +
> +        until = now(CLOCK_REALTIME) + TIMEOUT_USEC;
> +        for (;;) {
> +                usec_t n;
> +
> +                for (;;) {
> +                        n = now(CLOCK_REALTIME);
> +                        if (n >= until)
> +                                goto finish;
> +
> +                        if (waitpid(-1, NULL, WNOHANG) <= 0)
> +                                break;
> +
> +                        if (--processes == 0)
> +                                goto finish;
> +                }
> +
> +                timespec_store(&ts, until - n);
> +                sigtimedwait(&mask, NULL, &ts);

Please handle errors here. Write a log message at least. 

> +        }
> +
> +finish:
> +        return processes;
> +}
> +
> +int main(int argc, char *argv[]) {
> +        DIR *dir;
> +        int cmd;
> +        const char *args[5] = {KEXEC_BINARY_PATH, "-e", "-x", "-f", NULL};
> +
> +        if (getpid() != 1)
> +                return EXIT_FAILURE;
> +
> +        if (argc != 2)
> +                return EXIT_FAILURE;

Please add explanatory log messages here.

> +
> +        log_set_target(LOG_TARGET_SYSLOG_OR_KMSG);
> +        log_parse_environment();
> +        log_open();
> +
> +        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;
> +
> +        /* lock us into memory */
> +        mlockall(MCL_CURRENT|MCL_FUTURE);
> +
> +        /* opening /proc to read all processes */
> +        if ((dir = opendir("/proc")) == NULL)
> +                return EXIT_FAILURE;
> +
> +        /* sending SIGTERM and SIGKILL to all processes */
> +        kill(-1, SIGSTOP);
> +        log_info("Sending SIGTERM To Processes");
> +        if ((send_signal(dir, SIGTERM)) > 0) {
> +                log_info("Sending SIGKILL To Processes");
> +                send_signal(dir, SIGKILL);
> +        }
> +        kill(-1, SIGCONT);
> +
> +        /* preventing that we won't block umounts */
> +        if (chdir("/") == -1)
> +                return EXIT_FAILURE;
> +
> +        /* umount all mountpoints, swaps, dm and loops */
> +        log_info("Unmounting Filesystems");
> +        umount_all();
> +        log_info("Disabling Swaps");
> +        swapoff_all();
> +        /* FIXME:
> +         *  - undm_all()
> +         *  - unloop_all()
> +         *  - handling devicemapper
> +         */

You need a loop here. i.e. a swap you drop might make it possible to
unmount another fs (because the swap was about a swap file, not a swap
device). And so on.

> +
> +        sync();
> +
> +        if (cmd == RB_AUTOBOOT)
> +                execv(args[0], (char * const *) args);

Please add a proper verb above so that we can distuingish kexec and
normal reboots. (We probably want to have this all the way so that
there's a first klass kexec target and everything.

> +
> +        return reboot(cmd);
> +}

reboot() returns -1 on failure, which is not suitable for retruning from main().

Otherwise looks good!

Lennart

-- 
Lennart Poettering - Red Hat, Inc.