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). --- src/lxc/lxc_controller.c | 6 +++--- src/lxc/lxc_driver.c | 4 ++-- src/network/bridge_driver.c | 5 +++-- src/qemu/qemu_process.c | 2 +- src/util/command.c | 2 +- src/util/util.c | 30 +++++++++++++++--------------- tests/commandtest.c | 4 ++-- 7 files changed, 27 insertions(+), 26 deletions(-) diff --git a/src/lxc/lxc_controller.c b/src/lxc/lxc_controller.c index 7eda7ef..99d9467 100644 --- 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; /* rc is 0 or negative, but returns from main should be >=0 */ } diff --git a/src/lxc/lxc_driver.c b/src/lxc/lxc_driver.c index 615d2c6..7d99d27 100644 --- 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 f242db8..b2b0b06 100644 --- a/src/network/bridge_driver.c +++ b/src/network/bridge_driver.c @@ -756,8 +756,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 646215e..b87c320 100644 --- 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 29ccaa4..475eb62 100644 --- 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 d83215c..03a9e1a 100644 --- 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 9ab446c..ef2850d 100644 --- 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; } -- 1.7.3.4