On Fri, Apr 25, 2014 at 08:07:29PM +0200, Tom Gundersen wrote:
> On Fri, Apr 11, 2014 at 2:45 AM, Djalal Harouni <tix...@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.
Ok
> > ---
> > 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.
Yes, but in this case in all the other paths it will be
CONTAINER_TERMINATED, so I just avoided the:
if (!x) x = 1;
And here it doesn't matter if we return successfully, since on errors
the container will also be considered terminated. So I didn't want to
change the semantics.
> > + 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).
Yes, thanks, will do.
> > + if (status.si_code == CLD_EXITED) {
>
> Wouldn't this be better as a big switch?
Ok I'll try it.
> > + 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?
Please see below
> > + }
> > +
> > + 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).
Usually I find it easy to read, if there is a one final return for the
multiple branches. So I'll keep your second suggestion.
Ok, Tom will update and re-send, (sorry for the late response).
> > + } 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@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/systemd-devel
--
Djalal Harouni
http://opendz.org
_______________________________________________
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-devel
