[systemd-devel] [PATCH 5/8] power: these binaries are internal APIs

[Zbigniew Jędrzejewski-Szmek]

On Fri, Feb 20, 2015 at 02:31:02PM -0800, Shawn Landden wrote:
> They are not executed by a user (they all check how they were executed)
> so we can use assert() in main() just like we would anywhere else.
> ---
>  src/power/shutdown.c  | 20 ++++++++++----------
>  src/power/shutdownd.c | 22 ++++------------------
>  src/power/sleep.c     | 14 +++++++++++---
>  3 files changed, 25 insertions(+), 31 deletions(-)
> 
> diff --git a/src/power/shutdown.c b/src/power/shutdown.c
> index 71f001a..ffd12b8 100644
> --- a/src/power/shutdown.c
> +++ b/src/power/shutdown.c
> @@ -162,24 +162,24 @@ int main(int argc, char *argv[]) {
>          int cmd, r;
>          static const char* const dirs[] = {SYSTEM_SHUTDOWN_PATH, NULL};
>  
> -        log_parse_environment();
> -        r = parse_argv(argc, argv);
> -        if (r < 0)
> -                goto error;
> +        /* we are executed by execve() (without fork())
> +         * pid 1's main() at the bottom of src/core/main.c
> +         */
> +        assert(getpid() == 1);
No. assert is veryfing internal state, not for checking state that can
be influenced externally. In addition asserts can be compiled out, but
those checks should be the same for debug/non-debug builds.

Also, I don't really see the point of removing stuff which is already
written. Even if those binaries are not supposed to be called by hand,
when something goes wrong I'd much rather see a hand-written message
than a generic assert crash. assert() causes a coredump, which is
useless, since the problem was with the caller, and not the internal
state of the callee.

Zbyszek


> +        log_set_target(LOG_TARGET_AUTO);
> +        log_parse_environment();
>          /* journald will die if not gone yet. The log target defaults
>           * to console, but may have been changed by command line options. */
> -
>          log_close_console(); /* force reopen of /dev/console */
>          log_open();
>  
> +        r = parse_argv(argc, argv);
> +        assert(r >= 0);
> +
>          umask(0022);
>  
> -        if (getpid() != 1) {
> -                log_error("Not executed by init (PID 1).");
> -                r = -EPERM;
> -                goto error;
> -        }
> +        in_container = detect_container(NULL) > 0;
>  
>          if (streq(arg_verb, "reboot"))
>                  cmd = RB_AUTOBOOT;
> diff --git a/src/power/shutdownd.c b/src/power/shutdownd.c
> index 60a6468..742e1d5 100644
> --- a/src/power/shutdownd.c
> +++ b/src/power/shutdownd.c
> @@ -264,15 +264,9 @@ int main(int argc, char *argv[]) {
>          bool exec_shutdown = false, unlink_nologin = false;
>          unsigned i;
>  
> -        if (getppid() != 1) {
> -                log_error("This program should be invoked by init only.");
> -                return EXIT_FAILURE;
> -        }
> -
> -        if (argc > 1) {
> -                log_error("This program does not take arguments.");
> -                return EXIT_FAILURE;
> -        }
> +        /* we are executed through systemd-shutdownd.socket->systemd-shutdownd.service */
> +        assert(getppid() == 1);
> +        assert(argc == 0);
>  
>          log_set_target(LOG_TARGET_AUTO);
>          log_parse_environment();
> @@ -281,15 +275,7 @@ int main(int argc, char *argv[]) {
>          umask(0022);
>  
>          n_fds = sd_listen_fds(true);
> -        if (n_fds < 0) {
> -                log_error_errno(r, "Failed to read listening file descriptors from environment: %m");
> -                return EXIT_FAILURE;
> -        }
> -
> -        if (n_fds != 1) {
> -                log_error("Need exactly one file descriptor.");
> -                return EXIT_FAILURE;
> -        }
> +        assert(n_fds == 1);
>  
>          pollfd[FD_SOCKET].fd = SD_LISTEN_FDS_START;
>          pollfd[FD_SOCKET].events = POLLIN;
> diff --git a/src/power/sleep.c b/src/power/sleep.c
> index cc1ffa6..2462082 100644
> --- a/src/power/sleep.c
> +++ b/src/power/sleep.c
> @@ -200,17 +200,25 @@ int main(int argc, char *argv[]) {
>          _cleanup_strv_free_ char **modes = NULL, **states = NULL;
>          int r;
>  
> +        /* we are executed through
> +         *   - systemd-suspend.service
> +         *   - systemd-hibernate.service
> +         *   - systemd-hybrid-sleep.service
> +         */
> +        assert(getppid() == 1);
> +
>          log_set_target(LOG_TARGET_AUTO);
>          log_parse_environment();
>          log_open();
>  
>          r = parse_argv(argc, argv);
> -        if (r <= 0)
> -                goto finish;
> +        assert(r > 0);
>  
>          r = parse_sleep_config(arg_verb, &modes, &states);
> -        if (r < 0)
> +        if (r < 0) {
> +                log_error_errno(r, "Failed to parse " PKGSYSCONFDIR "/sleep.conf: %m");
>                  goto finish;
> +        }
>  
>          r = execute(modes, states);
>  
> -- 
> 2.2.1.209.g41e5f3a
> 
> _______________________________________________
> systemd-devel mailing list
> systemd-devel at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/systemd-devel