util: change virFile*Pid functions to return < 0 on failure

Although most functions in libvirt return 0 on success and < 0 on
failure, there are a few functions lingering around that return errno
(a positive value) on failure, and sometimes code calling those
functions incorrectly assumes the <0 standard. I noticed one of these
the other day when auditing networkStartDhcpDaemon after Guido Gunther
found a place where success was improperly returned on failure (that
patch has been acked and is pending a push). The problem was that it
expected the return value from virFileReadPid to be < 0 on failure,
but it was actually positive (it was also neglected to set the return
code in this case, similar to the bug found by Guido).

This all led to the fact that *all* of the virFile*Pid functions in
util.c are returning errno on failure. This patch remedies that
problem by changing them all to return -errno on failure, and makes
any necessary changes to callers of the functions. (In the meantime, I
also properly set the return code on failure of virFileReadPid in
networkStartDhcpDaemon).

diff --git a/src/lxc/lxc_controller.c b/src/lxc/lxc_controller.c
index 7eda7ef2761dc604243fadee45c3cfd53e0bf077..ff42aa569a7f6aac146900300584bb40032f10f0 100644 (file)
--- a/src/lxc/lxc_controller.c
+++ b/src/lxc/lxc_controller.c
@@ -947,8 +947,8 @@ int main(int argc, char *argv[])
             goto cleanup;
 
         if (pid > 0) {
-            if ((rc = virFileWritePid(LXC_STATE_DIR, name, pid)) != 0) {
-                virReportSystemError(rc,
+            if ((rc = virFileWritePid(LXC_STATE_DIR, name, pid)) < 0) {
+                virReportSystemError(-rc,
                                      _("Unable to write pid file '%s/%s.pid'"),
                                      LXC_STATE_DIR, name);
                 _exit(1);
@@ -996,5 +996,5 @@ cleanup:
         unlink(sockpath);
     VIR_FREE(sockpath);
 
-    return rc;
+    return rc ? EXIT_FAILURE : EXIT_SUCCESS;
 }

diff --git a/src/lxc/lxc_driver.c b/src/lxc/lxc_driver.c
index 615d2c647bfbda4178c2b519e58eec158b5d57d4..7d99d27ee8d800db7b0c28b78613cd17d3c80c0c 100644 (file)
--- a/src/lxc/lxc_driver.c
+++ b/src/lxc/lxc_driver.c
@@ -1612,8 +1612,8 @@ static int lxcVmStart(virConnectPtr conn,
         goto cleanup;
 
     /* And get its pid */
-    if ((r = virFileReadPid(driver->stateDir, vm->def->name, &vm->pid)) != 0) {
-        virReportSystemError(r,
+    if ((r = virFileReadPid(driver->stateDir, vm->def->name, &vm->pid)) < 0) {
+        virReportSystemError(-r,
                              _("Failed to read pid file %s/%s.pid"),
                              driver->stateDir, vm->def->name);
         goto cleanup;

diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c
index a21b538a5d37e548c59e9dcd6e26629c1d8e5534..b1c6b12bf38c92787279538d230094f3fa58efc5 100644 (file)
--- a/src/network/bridge_driver.c
+++ b/src/network/bridge_driver.c
@@ -758,8 +758,9 @@ networkStartDhcpDaemon(virNetworkObjPtr network)
      * pid
      */
 
-    if (virFileReadPid(NETWORK_PID_DIR, network->def->name,
-                       &network->dnsmasqPid) < 0)
+    ret = virFileReadPid(NETWORK_PID_DIR, network->def->name,
+                         &network->dnsmasqPid);
+    if (ret < 0)
         goto cleanup;
 
     ret = 0;

diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
index 646215e0ac44e670eed61f8ee9ed8cc66c437e68..b87c32060fcb5adb00464261da25c6981cad9da2 100644 (file)
--- a/src/qemu/qemu_process.c
+++ b/src/qemu/qemu_process.c
@@ -2754,7 +2754,7 @@ int qemuProcessStart(virConnectPtr conn,
 
     /* wait for qemu process to show up */
     if (ret == 0) {
-        if (virFileReadPidPath(priv->pidfile, &vm->pid)) {
+        if (virFileReadPidPath(priv->pidfile, &vm->pid) < 0) {
             qemuReportError(VIR_ERR_INTERNAL_ERROR,
                             _("Domain %s didn't show up"), vm->def->name);
             ret = -1;

diff --git a/src/util/command.c b/src/util/command.c
index 29ccaa4d7feafe7f8b91640614b8ea21466622c1..475eb62857e735c39a1c3bba2c7a9cb64871e01c 100644 (file)
--- a/src/util/command.c
+++ b/src/util/command.c
@@ -493,7 +493,7 @@ virExecWithHook(const char *const*argv,
         }
 
         if (pid > 0) {
-            if (pidfile && virFileWritePidPath(pidfile,pid)) {
+            if (pidfile && (virFileWritePidPath(pidfile,pid) < 0)) {
                 kill(pid, SIGTERM);
                 usleep(500*1000);
                 kill(pid, SIGTERM);

diff --git a/src/util/util.c b/src/util/util.c
index d83215ccc0fe0f28fa041886b567a033107e5b80..03a9e1adcdd2e307118dbaf690ceecede25f8fd3 100644 (file)
--- a/src/util/util.c
+++ b/src/util/util.c
@@ -1165,17 +1165,17 @@ int virFileWritePid(const char *dir,
     char *pidfile = NULL;
 
     if (name == NULL || dir == NULL) {
-        rc = EINVAL;
+        rc = -EINVAL;
         goto cleanup;
     }
 
     if (virFileMakePath(dir) < 0) {
-        rc = errno;
+        rc = -errno;
         goto cleanup;
     }
 
     if (!(pidfile = virFilePid(dir, name))) {
-        rc = ENOMEM;
+        rc = -ENOMEM;
         goto cleanup;
     }
 
@@ -1196,18 +1196,18 @@ int virFileWritePidPath(const char *pidfile,
     if ((fd = open(pidfile,
                    O_WRONLY | O_CREAT | O_TRUNC,
                    S_IRUSR | S_IWUSR)) < 0) {
-        rc = errno;
+        rc = -errno;
         goto cleanup;
     }
 
     if (!(file = VIR_FDOPEN(fd, "w"))) {
-        rc = errno;
+        rc = -errno;
         VIR_FORCE_CLOSE(fd);
         goto cleanup;
     }
 
     if (fprintf(file, "%d", pid) < 0) {
-        rc = errno;
+        rc = -errno;
         goto cleanup;
     }
 
@@ -1215,7 +1215,7 @@ int virFileWritePidPath(const char *pidfile,
 
 cleanup:
     if (VIR_FCLOSE(file) < 0)
-        rc = errno;
+        rc = -errno;
 
     return rc;
 }
@@ -1230,18 +1230,18 @@ int virFileReadPidPath(const char *path,
     *pid = 0;
 
     if (!(file = fopen(path, "r"))) {
-        rc = errno;
+        rc = -errno;
         goto cleanup;
     }
 
     if (fscanf(file, "%d", pid) != 1) {
-        rc = EINVAL;
+        rc = -EINVAL;
         VIR_FORCE_FCLOSE(file);
         goto cleanup;
     }
 
     if (VIR_FCLOSE(file) < 0) {
-        rc = errno;
+        rc = -errno;
         goto cleanup;
     }
 
@@ -1261,12 +1261,12 @@ int virFileReadPid(const char *dir,
     *pid = 0;
 
     if (name == NULL || dir == NULL) {
-        rc = EINVAL;
+        rc = -EINVAL;
         goto cleanup;
     }
 
     if (!(pidfile = virFilePid(dir, name))) {
-        rc = ENOMEM;
+        rc = -ENOMEM;
         goto cleanup;
     }
 
@@ -1284,17 +1284,17 @@ int virFileDeletePid(const char *dir,
     char *pidfile = NULL;
 
     if (name == NULL || dir == NULL) {
-        rc = EINVAL;
+        rc = -EINVAL;
         goto cleanup;
     }
 
     if (!(pidfile = virFilePid(dir, name))) {
-        rc = ENOMEM;
+        rc = -ENOMEM;
         goto cleanup;
     }
 
     if (unlink(pidfile) < 0 && errno != ENOENT)
-        rc = errno;
+        rc = -errno;
 
 cleanup:
     VIR_FREE(pidfile);

diff --git a/tests/commandtest.c b/tests/commandtest.c
index 9ab446c3630f6d971aa38d16a9af93817f059d58..ef2850d2575515e4ecfb0422a8c13a589362dedc 100644 (file)
--- a/tests/commandtest.c
+++ b/tests/commandtest.c
@@ -230,7 +230,7 @@ static int test4(const void *unused ATTRIBUTE_UNUSED)
         goto cleanup;
     }
 
-    if (virFileReadPid(abs_builddir, "commandhelper", &pid) != 0) {
+    if (virFileReadPid(abs_builddir, "commandhelper", &pid) < 0) {
         printf("cannot read pidfile\n");
         goto cleanup;
     }
@@ -686,7 +686,7 @@ static int test18(const void *unused ATTRIBUTE_UNUSED)
     }
     alarm(0);
 
-    if (virFileReadPid(abs_builddir, "commandhelper", &pid) != 0) {
+    if (virFileReadPid(abs_builddir, "commandhelper", &pid) < 0) {
         printf("cannot read pidfile\n");
         goto cleanup;
     }