[systemd-devel] [PATCH 1/3] nspawn: move container wait logic into wait_for_container() function

[Tom Gundersen]

On Fri, Apr 11, 2014 at 2:45 AM, Djalal Harouni <tixxdz at opendz.org> wrote:
> Move the container wait logic into its own wait_for_container() function
> and add two status codes: CONTAINER_TERMINATED or CONTAINER_REBOOTED
>
> These status codes are used to terminate nspawn or loop again in case of
> CONTAINER_REBOOTED.

Looks good, but some nit-picks below.

> ---
>  src/nspawn/nspawn.c | 114 ++++++++++++++++++++++++++++++++++------------------
>  1 file changed, 75 insertions(+), 39 deletions(-)
>
> diff --git a/src/nspawn/nspawn.c b/src/nspawn/nspawn.c
> index 0bd52da..d606bf2 100644
> --- a/src/nspawn/nspawn.c
> +++ b/src/nspawn/nspawn.c
> @@ -92,6 +92,11 @@
>  #include "seccomp-util.h"
>  #endif
>
> +typedef enum ContainerStatus {
> +        CONTAINER_TERMINATED,
> +        CONTAINER_REBOOTED
> +} ContainerStatus;
> +
>  typedef enum LinkJournal {
>          LINK_NO,
>          LINK_AUTO,
> @@ -2565,6 +2570,72 @@ static int change_uid_gid(char **_home) {
>          return 0;
>  }
>
> +/* Return 0 in case the container is being rebooted, has been shut
> + * down or exited succesfully. On failures a non-zero value is
> + * returned.
> + *
> + * The status of the container "CONTAINER_TERMINATED" or
> + * "CONTAINER_REBOOTED" will be saved in the container argument */
> +static int wait_for_container(pid_t pid, ContainerStatus *container) {
> +        int r, k;
> +        siginfo_t status;
> +
> +        /* Explicitly set this to CONTAINER_TERMINATED. If the reboot
> +         * conditions are met it will be updated to CONTAINER_REBOOTED */
> +        *container = CONTAINER_TERMINATED;

We don't usually clobber the arguments unless we return successfully,
so maybe delay this until we return.

> +        r = EXIT_FAILURE;
> +        k = wait_for_terminate(pid, &status);
> +        if (k < 0)
> +                return r;

Maybe better to just return k here, as EXIT_FAILURE is usually only
used in main() (also I think you can then drop k and just use r).

> +        if (status.si_code == CLD_EXITED) {

Wouldn't this be better as a big switch?

> +                r = status.si_status;
> +                if (status.si_status != 0) {
> +                        log_error("Container %s failed with error code %i.",
> +                                  arg_machine, status.si_status);
> +                        goto out;

Why not just "return status.si_status" and drop the out: label?

> +                }
> +
> +                if (!arg_quiet)
> +                        log_debug("Container %s exited successfully.",
> +                                  arg_machine);
> +
> +        } else if (status.si_code == CLD_KILLED &&
> +                   status.si_status == SIGINT) {
> +
> +                if (!arg_quiet)
> +                        log_info("Container %s has been shut down.",
> +                                 arg_machine);
> +                r = 0;

Same, just return directly, or let it fall through to a final "return
0" (and set *container here).

> +        } else if (status.si_code == CLD_KILLED &&
> +                   status.si_status == SIGHUP) {
> +
> +                if (!arg_quiet)
> +                        log_info("Container %s is being rebooted.",
> +                                 arg_machine);
> +                r = 0;
> +                *container = CONTAINER_REBOOTED;
> +
> +        } else if (status.si_code == CLD_KILLED ||
> +                   status.si_code == CLD_DUMPED) {
> +
> +                log_error("Container %s terminated by signal %s.",
> +                          arg_machine, signal_to_string(status.si_status));
> +                r = EXIT_FAILURE;
> +
> +        } else {
> +                log_error("Container %s failed due to unknown reason.",
> +                          arg_machine);
> +                r = EXIT_FAILURE;
> +        }
> +
> +out:
> +        return r;
> +}
> +
>  int main(int argc, char *argv[]) {
>
>          _cleanup_free_ char *kdbus_domain = NULL, *device_path = NULL, *root_device = NULL, *home_device = NULL, *srv_device = NULL;
> @@ -2743,8 +2814,8 @@ int main(int argc, char *argv[]) {
>          assert_se(sigprocmask(SIG_BLOCK, &mask, NULL) == 0);
>
>          for (;;) {
> +                ContainerStatus container_status;
>                  int parent_ready_fd = -1, child_ready_fd = -1;
> -                siginfo_t status;
>                  eventfd_t x;
>
>                  parent_ready_fd = eventfd(0, EFD_CLOEXEC);
> @@ -3094,48 +3165,13 @@ int main(int argc, char *argv[]) {
>                  /* Redundant, but better safe than sorry */
>                  kill(pid, SIGKILL);
>
> -                k = wait_for_terminate(pid, &status);
> +                r = wait_for_container(pid, &container_status);
>                  pid = 0;
>
> -                if (k < 0) {
> -                        r = EXIT_FAILURE;
> -                        break;
> -                }
> -
> -                if (status.si_code == CLD_EXITED) {
> -                        r = status.si_status;
> -                        if (status.si_status != 0) {
> -                                log_error("Container %s failed with error code %i.", arg_machine, status.si_status);
> -                                break;
> -                        }
> -
> -                        if (!arg_quiet)
> -                                log_debug("Container %s exited successfully.", arg_machine);
> +                if (r || container_status == CONTAINER_TERMINATED)
>                          break;
> -                } else if (status.si_code == CLD_KILLED &&
> -                           status.si_status == SIGINT) {
>
> -                        if (!arg_quiet)
> -                                log_info("Container %s has been shut down.", arg_machine);
> -                        r = 0;
> -                        break;
> -                } else if (status.si_code == CLD_KILLED &&
> -                           status.si_status == SIGHUP) {
> -
> -                        if (!arg_quiet)
> -                                log_info("Container %s is being rebooted.", arg_machine);
> -                        continue;
> -                } else if (status.si_code == CLD_KILLED ||
> -                           status.si_code == CLD_DUMPED) {
> -
> -                        log_error("Container %s terminated by signal %s.", arg_machine, signal_to_string(status.si_status));
> -                        r = EXIT_FAILURE;
> -                        break;
> -                } else {
> -                        log_error("Container %s failed due to unknown reason.", arg_machine);
> -                        r = EXIT_FAILURE;
> -                        break;
> -                }
> +                /* CONTAINER_REBOOTED, loop again */
>          }
>
>  finish:
> --
> 1.8.5.3
>
> _______________________________________________
> systemd-devel mailing list
> systemd-devel at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/systemd-devel