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
_______________________________________________
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-devel
