[libvirt] [PATCH 0/3] more virCommand cleanups
When I first reviewed Cole's virCommand cleanups, I pointed out some possible later cleanups. Here they are. Eric Blake (3): command: reduce duplicated debug messages command: avoid double close storage: avoid an intermediate malloc src/storage/storage_backend.c | 20 ++++++------ src/util/command.c | 64 ++++++++++++++++++++-------------------- 2 files changed, 42 insertions(+), 42 deletions(-) -- 1.7.4.4
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
2011/6/14 Eric Blake <eblake@redhat.com>:
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
ACK. -- Matthias Bolte http://photron.blogspot.com
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
2011/6/14 Eric Blake <eblake@redhat.com>:
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(-)
ACK. -- Matthias Bolte http://photron.blogspot.com
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
2011/6/14 Eric Blake <eblake@redhat.com>:
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(-)
ACK. -- Matthias Bolte http://photron.blogspot.com
On 06/14/2011 04:24 AM, Matthias Bolte wrote:
2011/6/14 Eric Blake <eblake@redhat.com>:
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(-)
ACK.
Thanks; series pushed. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
participants (2)
-
Eric Blake -
Matthias Bolte