From 631923e7f2637c4b74e2e47918e327f87cb1e1a4 Mon Sep 17 00:00:00 2001 From: Eric Blake Date: Tue, 18 Feb 2014 18:06:50 -0700 Subject: [PATCH] virFork: give specific status on failure prior to exec When a child fails without exec'ing, we want a well-known status; best is to match what env(1), nice(1), su(1), and other wrapper programs do. This patch adds enum values that later patches will use, and sets up virFork as the first client of EXIT_CANCELED for errors detected prior to even attempting exec, as well as virExec to distinguish between a missing executable vs. a binary that cannot be executed. This is a slight semantic change in the unlikely case of a child process failing to restore its signal mask - we now kill the child with a known status instead of relying on the caller to notice and do an appropriate _exit(). A subsequent patch will make further cleanups based on an audit of all callers. * src/internal.h (EXIT_CANCELED, EXIT_CANNOT_INVOKE) (EXIT_ENOENT): New enum. * src/util/vircommand.c (virFork): Document specific exit value if child aborts early. (virExec): Distinguish between various exec failures. * tests/commandtest.c (test1): Enhance test. (test22): New test. Signed-off-by: Eric Blake --- src/internal.h | 7 +++++++ src/util/vircommand.c | 15 +++++++++------ tests/commandtest.c | 43 +++++++++++++++++++++++++++++++++++++++++-- 3 files changed, 57 insertions(+), 8 deletions(-) diff --git a/src/internal.h b/src/internal.h index cef3da0328..5a38448b16 100644 --- a/src/internal.h +++ b/src/internal.h @@ -438,5 +438,12 @@ #NAME ": " FMT, __VA_ARGS__); # endif +/* Specific error values for use in forwarding programs such as + * virt-login-shell; these values match what GNU env does. */ +enum { + EXIT_CANCELED = 125, /* Failed before attempting exec */ + EXIT_CANNOT_INVOKE = 126, /* Exists but couldn't exec */ + EXIT_ENOENT = 127, /* Could not find program to exec */ +}; #endif /* __VIR_INTERNAL_H__ */ diff --git a/src/util/vircommand.c b/src/util/vircommand.c index b137436ea6..b9e5f3748a 100644 --- a/src/util/vircommand.c +++ b/src/util/vircommand.c @@ -1,7 +1,7 @@ /* * vircommand.c: Child command execution * - * Copyright (C) 2010-2013 Red Hat, Inc. + * Copyright (C) 2010-2014 Red Hat, Inc. * * This library is free software; you can redistribute it and/or * modify it under the terms of the GNU Lesser General Public @@ -199,7 +199,7 @@ virCommandFDSet(virCommandPtr cmd, * @pid - a pointer to a pid_t that will receive the return value from * fork() * - * fork a new process while avoiding various race/deadlock conditions + * Wrapper around fork() that avoids various race/deadlock conditions. * * on return from virFork(), if *pid < 0, the fork failed and there is * no new process. Otherwise, just like fork(), if *pid == 0, it is the @@ -208,7 +208,7 @@ virCommandFDSet(virCommandPtr cmd, * Even if *pid >= 0, if the return value from virFork() is < 0, it * indicates a failure that occurred in the parent or child process * after the fork. In this case, the child process should call - * _exit(EXIT_FAILURE) after doing any additional error reporting. + * _exit(EXIT_CANCELED) after doing any additional error reporting. */ int virFork(pid_t *pid) @@ -304,7 +304,8 @@ virFork(pid_t *pid) if (pthread_sigmask(SIG_SETMASK, &newmask, NULL) != 0) { saved_errno = errno; /* save for caller */ virReportSystemError(errno, "%s", _("cannot unblock signals")); - goto cleanup; + virDispatchError(NULL); + _exit(EXIT_CANCELED); } ret = 0; } @@ -518,6 +519,7 @@ virExec(virCommandPtr cmd) /* child */ + ret = EXIT_CANCELED; if (forkRet < 0) { /* The fork was successful, but after that there was an error * in the child (which was already logged). @@ -603,7 +605,7 @@ virExec(virCommandPtr cmd) cmd->pidfile, pid); goto fork_error; } - _exit(0); + _exit(EXIT_SUCCESS); } } @@ -703,13 +705,14 @@ virExec(virCommandPtr cmd) else execv(binary, cmd->args); + ret = errno == ENOENT ? EXIT_ENOENT : EXIT_CANNOT_INVOKE; virReportSystemError(errno, _("cannot execute binary %s"), cmd->args[0]); fork_error: virDispatchError(NULL); - _exit(EXIT_FAILURE); + _exit(ret); cleanup: /* This is cleanup of parent process only - child diff --git a/tests/commandtest.c b/tests/commandtest.c index 2ae8871fd1..042f04959d 100644 --- a/tests/commandtest.c +++ b/tests/commandtest.c @@ -1,7 +1,7 @@ /* * commandtest.c: Test the libCommand API * - * Copyright (C) 2010-2013 Red Hat, Inc. + * Copyright (C) 2010-2014 Red Hat, Inc. * * This library is free software; you can redistribute it and/or * modify it under the terms of the GNU Lesser General Public @@ -140,7 +140,7 @@ static int test1(const void *unused ATTRIBUTE_UNUSED) cmd = virCommandNew(abs_builddir "/commandhelper-doesnotexist"); if (virCommandRun(cmd, &status) < 0) goto cleanup; - if (status == 0) + if (!WIFEXITED(status) || WEXITSTATUS(status) != EXIT_ENOENT) goto cleanup; ret = 0; @@ -899,6 +899,44 @@ cleanup: return ret; } +static int +test22(const void *unused ATTRIBUTE_UNUSED) +{ + int ret = -1; + virCommandPtr cmd; + int status = -1; + + cmd = virCommandNewArgList("/bin/sh", "-c", "exit 3", NULL); + + if (virCommandRun(cmd, &status) < 0) { + virErrorPtr err = virGetLastError(); + printf("Cannot run child %s\n", err->message); + goto cleanup; + } + if (!WIFEXITED(status) || WEXITSTATUS(status) != 3) { + printf("Unexpected status %d\n", status); + goto cleanup; + } + + virCommandFree(cmd); + cmd = virCommandNewArgList("/bin/sh", "-c", "kill -9 $$", NULL); + + if (virCommandRun(cmd, &status) < 0) { + virErrorPtr err = virGetLastError(); + printf("Cannot run child %s\n", err->message); + goto cleanup; + } + if (!WIFSIGNALED(status) || WTERMSIG(status) != SIGKILL) { + printf("Unexpected status %d\n", status); + goto cleanup; + } + + ret = 0; +cleanup: + virCommandFree(cmd); + return ret; +} + static void virCommandThreadWorker(void *opaque) { virCommandTestDataPtr test = opaque; @@ -1046,6 +1084,7 @@ mymain(void) DO_TEST(test19); DO_TEST(test20); DO_TEST(test21); + DO_TEST(test22); virMutexLock(&test->lock); if (test->running) { -- GitLab