[systemd-devel] [PATCH] nspawn: When exiting with an error, make the error code meaningful.

[Djalal Harouni]

Hi,

You are right, commit 113cea8 introduced this regression, we guarantee
that nspawn returns the exit code of the program executed in the
container in case nspwan wont fail. My bad, I missed this point...
sorry!

Ok will comment on this patch, the other one is wrong, since we mix
-errno with exit status, 'status.si_status' contains the low 8 bits of
the value returned by child, in other words it is equivalent to
WIFEXITED() of waitpid(), so that patch converts any exit status to an
-errno code...


IMHO this is the right patch.

On Sat, Jun 28, 2014 at 12:09:56PM -0400, Luke Shumaker wrote:
> This is accomplished by having wait_for_container() return a positive error
> code when we would like that error code to make its way to the user.  This
> is at odds with the CODING_STYLE rule that error codes should be returned
> as negative values.
This is not odd, we already do that!

When I did convert to wait_for_container(), I remember I checked all
calls to wait_for_terminate() to see what others do, and there was the:
src/shared/util.c:wait_for_terminate_and_warn()

Which also returns the positive 'status.si_status' code to caller, and it
is used in all places...

I checked that function, but was played by the fact that if the child
fails to setup the container we just fail with _exit(EXIT_FAILURE), no
specific code, so I converted to -1 and introduced the regression... :-/


So if you are able to send a v2 of this patch, here are some comments:

Please improve the commit log, note the commit that caused the regression,
and remove the mention to CODING_STYLE, since we already do this in
systemd, and for nspawn: the positive code returned by
wait_for_container() is the exit code of the program executed in the
container, it can be positive or EXIT_SUCCESS, and this is already
documented.

It will be easy to buy it this way :-)

> ---
>  src/nspawn/nspawn.c | 21 +++++++++++++--------
>  1 file changed, 13 insertions(+), 8 deletions(-)
> 
> diff --git a/src/nspawn/nspawn.c b/src/nspawn/nspawn.c
> index 0a8dc0c..42f939b 100644
> --- a/src/nspawn/nspawn.c
> +++ b/src/nspawn/nspawn.c
> @@ -2645,12 +2645,17 @@ static int change_uid_gid(char **_home) {
>  }
>  
>  /*
> - * Return 0 in case the container is being rebooted, has been shut
> - * down or exited successfully. On failures a negative value is
> - * returned.
> + * Return values:
> + * < 0 : The container was terminated by a signal, or failed for an
> + *       unknown reason.  No change is made to the container argument.
wait_for_container() failed to get the state of the container, the
container was terminated by a signal or failed for an unknown reason.



> + * > 0 : The container terminated with an error code, which is the
> + *       return value.  No change is made to the container argument.
"The exit code of the program executed in the container is returned."

Quoted from systemd-nspawn man page.


> + *   0 : The container is being rebooted, has been shut down or exited
> + *       successfully.  The container argument has been set to either
> + *       CONTAINER_TERMINATED or CONTAINER_REBOOTED.
Nice!

>   *
> - * The status of the container "CONTAINER_TERMINATED" or
> - * "CONTAINER_REBOOTED" will be saved in the container argument
> + * That is, success is indicated by a return value of zero, and an
> + * error is indicated by a non-zero value.
>   */
>  static int wait_for_container(pid_t pid, ContainerStatus *container) {
>          int r;
> @@ -2672,7 +2677,6 @@ static int wait_for_container(pid_t pid, ContainerStatus *container) {
A minor thing:

in wait_for_container() could you add please a log_warning() if
wait_for_terminate() fails, as it is done in
wait_for_terminate_and_warn().


>                  } else {
>                          log_error("Container %s failed with error code %i.",
>                                    arg_machine, status.si_status);
> -                        r = -1;
>                  }
>                  break;
>  
> @@ -3299,8 +3303,9 @@ check_container_status:
>                  r = wait_for_container(pid, &container_status);
>                  pid = 0;
>  
> -                if (r < 0) {
> -                        r = EXIT_FAILURE;
> +                if (r != 0) {
Add a code comment, if (r < 0) explicitly set to EXIT_FAILURE, otherwise
return the exit code of the containered process.

> +                        if (r < 0)
> +                                r = EXIT_FAILURE;
>                          break;
>                  } else if (container_status == CONTAINER_TERMINATED)
>                          break;

Thank you for the report and the patch!

-- 
Djalal Harouni
http://opendz.org