This also reduces malloc pressure for invoking a child when VIR_DEBUG is enabled. * src/util/command.c (virExecWithHook): Drop debug, since the only caller (virCommandRunAsync) also prints debug info. --- Suggested here: https://www.redhat.com/archives/libvir-list/2011-May/msg00592.html src/util/command.c | 20 -------------------- 1 files changed, 0 insertions(+), 20 deletions(-) diff --git a/src/util/command.c b/src/util/command.c index 0f8ff14..1144392 100644 --- a/src/util/command.c +++ b/src/util/command.c @@ -296,26 +296,6 @@ virExecWithHook(const char *const*argv, int tmpfd; const char *binary = NULL; int forkRet; - char *argv_str = NULL; - char *envp_str = NULL; - - if ((argv_str = virArgvToString(argv)) == NULL) { - virReportOOMError(); - return -1; - } - - if (envp) { - if ((envp_str = virArgvToString(envp)) == NULL) { - VIR_FREE(argv_str); - virReportOOMError(); - return -1; - } - VIR_DEBUG("%s %s", envp_str, argv_str); - VIR_FREE(envp_str); - } else { - VIR_DEBUG("%s", argv_str); - } - VIR_FREE(argv_str); if (argv[0][0] != '/') { if (!(binary = virFindFileInPath(argv[0]))) { -- 1.7.4.4

Previously, the parent process opened 'null' to /dev/null, then the child process closes 'null' as well as 'childout'. But if childout was set to be null, then this is a double close. At least the double close was confined to the child process after a fork, and therefore there is no risk of another thread opening an fd of the same value to be bitten by the double close, but it is always better to avoid double-close to begin with. Additionally, if all three fds were specified, then opening 'null' was wasted. This patch fixes things to lazily open null on the first use, then guarantees it gets closed exactly once. * src/util/command.c (getDevNull): New helper function. (virExecWithHook): Use it to avoid spurious opens and double close. --- src/util/command.c | 44 ++++++++++++++++++++++++++++++++------------ 1 files changed, 32 insertions(+), 12 deletions(-) diff --git a/src/util/command.c b/src/util/command.c index 1144392..cb682fc 100644 --- a/src/util/command.c +++ b/src/util/command.c @@ -258,6 +258,23 @@ cleanup: } /* + * Ensure that *null is an fd visiting /dev/null. Return 0 on + * success, -1 on failure. Allows for lazy opening of shared + * /dev/null fd only as required. + */ +static int +getDevNull(int *null) +{ + if (*null == -1 && (*null = open("/dev/null", O_RDWR)) < 0) { + virReportSystemError(errno, + _("cannot open %s"), + "/dev/null"); + return -1; + } + return 0; +} + +/* * @argv argv to exec * @envp optional environment to use for exec * @keepfd options fd_ret to keep open for child process @@ -288,7 +305,7 @@ virExecWithHook(const char *const*argv, char *pidfile) { pid_t pid; - int null, i, openmax; + int null = -1, i, openmax; int pipeout[2] = {-1,-1}; int pipeerr[2] = {-1,-1}; int childout = -1; @@ -308,11 +325,10 @@ virExecWithHook(const char *const*argv, binary = argv[0]; } - if ((null = open("/dev/null", O_RDWR)) < 0) { - virReportSystemError(errno, - _("cannot open %s"), - "/dev/null"); - goto cleanup; + if (infd < 0) { + if (getDevNull(&null) < 0) + goto cleanup; + infd = null; } if (outfd != NULL) { @@ -341,6 +357,8 @@ virExecWithHook(const char *const*argv, childout = *outfd; } } else { + if (getDevNull(&null) < 0) + goto cleanup; childout = null; } @@ -370,6 +388,8 @@ virExecWithHook(const char *const*argv, childerr = *errfd; } } else { + if (getDevNull(&null) < 0) + goto cleanup; childerr = null; } @@ -414,7 +434,6 @@ virExecWithHook(const char *const*argv, openmax = sysconf(_SC_OPEN_MAX); for (i = 3; i < openmax; i++) if (i != infd && - i != null && i != childout && i != childerr && (!keepfd || i >= FD_SETSIZE || !FD_ISSET(i, keepfd))) { @@ -422,7 +441,7 @@ virExecWithHook(const char *const*argv, VIR_FORCE_CLOSE(tmpfd); } - if (dup2(infd >= 0 ? infd : null, STDIN_FILENO) < 0) { + if (dup2(infd, STDIN_FILENO) < 0) { virReportSystemError(errno, "%s", _("failed to setup stdin file handle")); goto fork_error; @@ -440,17 +459,18 @@ virExecWithHook(const char *const*argv, goto fork_error; } - if (infd != STDIN_FILENO) + if (infd != STDIN_FILENO && infd != null) VIR_FORCE_CLOSE(infd); - VIR_FORCE_CLOSE(null); - if (childout > STDERR_FILENO) { + if (childout > STDERR_FILENO && childout != null) { tmpfd = childout; /* preserve childout value */ VIR_FORCE_CLOSE(tmpfd); } if (childerr > STDERR_FILENO && - childerr != childout) { + childerr != childout && + childerr != null) { VIR_FORCE_CLOSE(childerr); } + VIR_FORCE_CLOSE(null); /* Initialize full logging for a while */ virLogSetFromEnv(); -- 1.7.4.4

Suggested here: https://www.redhat.com/archives/libvir-list/2011-May/msg00594.html * src/storage/storage_backend.c (virStorageBackendCreateQemuImg): Generate size inline. --- src/storage/storage_backend.c | 20 ++++++++++---------- 1 files changed, 10 insertions(+), 10 deletions(-) diff --git a/src/storage/storage_backend.c b/src/storage/storage_backend.c index d269e4c..8d0ea9e 100644 --- a/src/storage/storage_backend.c +++ b/src/storage/storage_backend.c @@ -649,11 +649,11 @@ virStorageBackendCreateQemuImg(virConnectPtr conn, unsigned int flags ATTRIBUTE_UNUSED) { int ret = -1; - char *size = NULL; char *create_tool; int imgformat = -1; virCommandPtr cmd = NULL; bool do_encryption = (vol->target.encryption != NULL); + unsigned long long int size_arg; const char *type = virStorageFileFormatTypeToString(vol->target.format); const char *backingType = vol->backingStore.path ? @@ -757,10 +757,7 @@ virStorageBackendCreateQemuImg(virConnectPtr conn, } /* Size in KB */ - if (virAsprintf(&size, "%lluK", VIR_DIV_UP(vol->capacity, 1024)) < 0) { - virReportOOMError(); - goto cleanup; - } + size_arg = VIR_DIV_UP(vol->capacity, 1024); /* KVM is usually ahead of qemu on features, so try that first */ create_tool = virFindFileInPath("kvm-img"); @@ -798,7 +795,8 @@ virStorageBackendCreateQemuImg(virConnectPtr conn, switch (imgformat) { case QEMU_IMG_BACKING_FORMAT_FLAG: virCommandAddArgList(cmd, "-F", backingType, vol->target.path, - size, NULL); + NULL); + virCommandAddArgFormat(cmd, "%lluK", size_arg); if (do_encryption) virCommandAddArg(cmd, "-e"); @@ -808,20 +806,23 @@ virStorageBackendCreateQemuImg(virConnectPtr conn, virCommandAddArg(cmd, "-o"); virCommandAddArgFormat(cmd, "backing_fmt=%s%s", backingType, do_encryption ? ",encryption=on" : ""); - virCommandAddArgList(cmd, vol->target.path, size, NULL); + virCommandAddArg(cmd, vol->target.path); + virCommandAddArgFormat(cmd, "%lluK", size_arg); break; default: VIR_INFO("Unable to set backing store format for %s with %s", vol->target.path, create_tool); - virCommandAddArgList(cmd, vol->target.path, size, NULL); + virCommandAddArg(cmd, vol->target.path); + virCommandAddArgFormat(cmd, "%lluK", size_arg); if (do_encryption) virCommandAddArg(cmd, "-e"); } } else { virCommandAddArgList(cmd, "create", "-f", type, - vol->target.path, size, NULL); + vol->target.path, NULL); + virCommandAddArgFormat(cmd, "%lluK", size_arg); if (do_encryption) { if (imgformat == QEMU_IMG_BACKING_FORMAT_OPTIONS) { @@ -834,7 +835,6 @@ virStorageBackendCreateQemuImg(virConnectPtr conn, ret = virStorageBackendCreateExecCommand(pool, vol, cmd); cleanup: - VIR_FREE(size); VIR_FREE(create_tool); virCommandFree(cmd); -- 1.7.4.4