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 <eblake@redhat.com>

diff --git a/src/internal.h b/src/internal.h
index cef3da03282ac3a9c42d5ba06283d40ffeb7704a..5a38448b16d32f2872bd8a761873fb0bae213aa5 100644 (file)
--- 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 b137436ea6747f3c796590a19412c34315ed4922..b9e5f3748a87d135a9574636a9e8986a8e8ec768 100644 (file)
--- 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 2ae8871fd14fa699e4f49e797bf3a9cd9ed6a1b8..042f04959d67894f5de89f6f7c82eb4e77e7f944 100644 (file)
--- 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) {