[PATCH 0/6] virCommand cleanups
Some cleanups I've accumulated. Peter Krempa (6): virPipeImpl: Don't overwrite error virCommandAddEnvBuffer: Remove unused function util: vircommand: Add wrappers for virCommand error checking virCommandFDSet: Remove return value virCommandSetSendBuffer: Provide saner semantics commandtest: test27: Remove pointless 'cleanup' label src/libvirt_private.syms | 1 - src/qemu/qemu_tpm.c | 50 ++------- src/util/vircommand.c | 230 ++++++++++++++++++--------------------- src/util/vircommand.h | 9 +- src/util/virutil.c | 2 - tests/commandtest.c | 49 ++------- 6 files changed, 131 insertions(+), 210 deletions(-) -- 2.29.2
If WITH_PIPE2 is not defined we attempt to set the pipe to nonblocking operation after they are created. We errorneously rewrote the existing error message on failure to do so or even reported an error if quiet mode was requested. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/util/virutil.c | 2 -- 1 file changed, 2 deletions(-) diff --git a/src/util/virutil.c b/src/util/virutil.c index a0cd0f1bcd..0f3cf98613 100644 --- a/src/util/virutil.c +++ b/src/util/virutil.c @@ -1924,8 +1924,6 @@ virPipeImpl(int fds[2], bool nonblock, bool errreport) if (errreport) virReportSystemError(errno, "%s", _("Unable to set pipes to non-blocking")); - virReportSystemError(errno, "%s", - _("Unable to create pipes")); VIR_FORCE_CLOSE(fds[0]); VIR_FORCE_CLOSE(fds[1]); return -1; -- 2.29.2
On a Tuesday in 2021, Peter Krempa wrote:
If WITH_PIPE2 is not defined we attempt to set the pipe to nonblocking operation after they are created. We errorneously rewrote the existing error message on failure to do so or even reported an error if quiet mode was requested.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Fixes: ab36f729470c313b9d5b7debdbeac441f7780dec
--- src/util/virutil.c | 2 -- 1 file changed, 2 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/libvirt_private.syms | 1 - src/util/vircommand.c | 26 -------------------------- src/util/vircommand.h | 3 --- 3 files changed, 30 deletions(-) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 2a3bbdc577..90c02a250c 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1965,7 +1965,6 @@ virCommandAddArgFormat; virCommandAddArgList; virCommandAddArgPair; virCommandAddArgSet; -virCommandAddEnvBuffer; virCommandAddEnvFormat; virCommandAddEnvPair; virCommandAddEnvPass; diff --git a/src/util/vircommand.c b/src/util/vircommand.c index 1a4b77ea24..28a903e117 100644 --- a/src/util/vircommand.c +++ b/src/util/vircommand.c @@ -1409,32 +1409,6 @@ virCommandAddEnvString(virCommandPtr cmd, const char *str) } -/** - * virCommandAddEnvBuffer: - * @cmd: the command to modify - * @buf: buffer that contains name=value string, which will be reset on return - * - * Convert a buffer containing preformatted name=value into an - * environment variable of the child. - * Correctly transfers memory errors or contents from buf to cmd. - */ -void -virCommandAddEnvBuffer(virCommandPtr cmd, virBufferPtr buf) -{ - if (!cmd || cmd->has_error) { - virBufferFreeAndReset(buf); - return; - } - - if (!virBufferUse(buf)) { - cmd->has_error = EINVAL; - return; - } - - virCommandAddEnv(cmd, virBufferContentAndReset(buf)); -} - - /** * virCommandAddEnvPass: * @cmd: the command to modify diff --git a/src/util/vircommand.h b/src/util/vircommand.h index 0ea6c8229f..9fb625ec4b 100644 --- a/src/util/vircommand.h +++ b/src/util/vircommand.h @@ -109,9 +109,6 @@ void virCommandAddEnvPair(virCommandPtr cmd, void virCommandAddEnvString(virCommandPtr cmd, const char *str) ATTRIBUTE_NONNULL(2); -void virCommandAddEnvBuffer(virCommandPtr cmd, - virBufferPtr buf); - void virCommandAddEnvPass(virCommandPtr cmd, const char *name) ATTRIBUTE_NONNULL(2); -- 2.29.2
On a Tuesday in 2021, Peter Krempa wrote:
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/libvirt_private.syms | 1 - src/util/vircommand.c | 26 -------------------------- src/util/vircommand.h | 3 --- 3 files changed, 30 deletions(-)
Last usage was removed in 2011 by: commit 5745dc123a4798db36dd0c78c764cc29a9cf71ce qemu/rbd: improve rbd device specification Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
Extract the check and reporting of error from the individual virCommand APIs into a separate helper. This will aid future refactors. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/util/vircommand.c | 143 ++++++++++++++++++++++-------------------- 1 file changed, 76 insertions(+), 67 deletions(-) diff --git a/src/util/vircommand.c b/src/util/vircommand.c index 28a903e117..2f6635671d 100644 --- a/src/util/vircommand.c +++ b/src/util/vircommand.c @@ -161,6 +161,27 @@ static void *dryRunOpaque; static int dryRunStatus; #endif /* !WIN32 */ + +static bool +virCommandHasError(virCommandPtr cmd) +{ + return !cmd || cmd->has_error != 0; +} + + +static int +virCommandRaiseError(virCommandPtr cmd) +{ + if (!cmd || cmd->has_error != 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("invalid use of command API")); + return -1; + } + + return 0; +} + + /* * virCommandFDIsSet: * @cmd: pointer to virCommand @@ -982,7 +1003,7 @@ virCommandNewVAList(const char *binary, va_list list) virCommandPtr cmd = virCommandNew(binary); const char *arg; - if (!cmd || cmd->has_error) + if (virCommandHasError(cmd)) return cmd; while ((arg = va_arg(list, const char *)) != NULL) @@ -1076,7 +1097,7 @@ virCommandPassFDGetFDIndex(virCommandPtr cmd, int fd) { size_t i = 0; - if (!cmd || cmd->has_error) + if (virCommandHasError(cmd)) return -1; while (i < cmd->npassfd) { @@ -1100,7 +1121,7 @@ virCommandPassFDGetFDIndex(virCommandPtr cmd, int fd) void virCommandSetPidFile(virCommandPtr cmd, const char *pidfile) { - if (!cmd || cmd->has_error) + if (virCommandHasError(cmd)) return; VIR_FREE(cmd->pidfile); @@ -1125,7 +1146,7 @@ virCommandGetUID(virCommandPtr cmd) void virCommandSetGID(virCommandPtr cmd, gid_t gid) { - if (!cmd || cmd->has_error) + if (virCommandHasError(cmd)) return; cmd->gid = gid; @@ -1134,7 +1155,7 @@ virCommandSetGID(virCommandPtr cmd, gid_t gid) void virCommandSetUID(virCommandPtr cmd, uid_t uid) { - if (!cmd || cmd->has_error) + if (virCommandHasError(cmd)) return; cmd->uid = uid; @@ -1143,7 +1164,7 @@ virCommandSetUID(virCommandPtr cmd, uid_t uid) void virCommandSetMaxMemLock(virCommandPtr cmd, unsigned long long bytes) { - if (!cmd || cmd->has_error) + if (virCommandHasError(cmd)) return; cmd->maxMemLock = bytes; @@ -1152,7 +1173,7 @@ virCommandSetMaxMemLock(virCommandPtr cmd, unsigned long long bytes) void virCommandSetMaxProcesses(virCommandPtr cmd, unsigned int procs) { - if (!cmd || cmd->has_error) + if (virCommandHasError(cmd)) return; cmd->maxProcesses = procs; @@ -1161,7 +1182,7 @@ virCommandSetMaxProcesses(virCommandPtr cmd, unsigned int procs) void virCommandSetMaxFiles(virCommandPtr cmd, unsigned int files) { - if (!cmd || cmd->has_error) + if (virCommandHasError(cmd)) return; cmd->maxFiles = files; @@ -1169,7 +1190,7 @@ virCommandSetMaxFiles(virCommandPtr cmd, unsigned int files) void virCommandSetMaxCoreSize(virCommandPtr cmd, unsigned long long bytes) { - if (!cmd || cmd->has_error) + if (virCommandHasError(cmd)) return; cmd->maxCore = bytes; @@ -1178,7 +1199,7 @@ void virCommandSetMaxCoreSize(virCommandPtr cmd, unsigned long long bytes) void virCommandSetUmask(virCommandPtr cmd, int mask) { - if (!cmd || cmd->has_error) + if (virCommandHasError(cmd)) return; cmd->mask = mask; @@ -1193,7 +1214,7 @@ void virCommandSetUmask(virCommandPtr cmd, int mask) void virCommandClearCaps(virCommandPtr cmd) { - if (!cmd || cmd->has_error) + if (virCommandHasError(cmd)) return; cmd->flags |= VIR_EXEC_CLEAR_CAPS; @@ -1210,7 +1231,7 @@ void virCommandAllowCap(virCommandPtr cmd, int capability) { - if (!cmd || cmd->has_error) + if (virCommandHasError(cmd)) return; cmd->capabilities |= (1ULL << capability); @@ -1231,7 +1252,7 @@ void virCommandSetSELinuxLabel(virCommandPtr cmd, const char *label G_GNUC_UNUSED) { - if (!cmd || cmd->has_error) + if (virCommandHasError(cmd)) return; #if defined(WITH_SECDRIVER_SELINUX) @@ -1255,7 +1276,7 @@ void virCommandSetAppArmorProfile(virCommandPtr cmd, const char *profile G_GNUC_UNUSED) { - if (!cmd || cmd->has_error) + if (virCommandHasError(cmd)) return; #if defined(WITH_SECDRIVER_APPARMOR) @@ -1277,7 +1298,7 @@ virCommandSetAppArmorProfile(virCommandPtr cmd, void virCommandDaemonize(virCommandPtr cmd) { - if (!cmd || cmd->has_error) + if (virCommandHasError(cmd)) return; cmd->flags |= VIR_EXEC_DAEMON; @@ -1293,7 +1314,7 @@ virCommandDaemonize(virCommandPtr cmd) void virCommandNonblockingFDs(virCommandPtr cmd) { - if (!cmd || cmd->has_error) + if (virCommandHasError(cmd)) return; cmd->flags |= VIR_EXEC_NONBLOCK; @@ -1312,7 +1333,7 @@ virCommandNonblockingFDs(virCommandPtr cmd) void virCommandRawStatus(virCommandPtr cmd) { - if (!cmd || cmd->has_error) + if (virCommandHasError(cmd)) return; cmd->rawStatus = true; @@ -1361,7 +1382,7 @@ virCommandAddEnvFormat(virCommandPtr cmd, const char *format, ...) char *env; va_list list; - if (!cmd || cmd->has_error) + if (virCommandHasError(cmd)) return; va_start(list, format); @@ -1400,7 +1421,7 @@ virCommandAddEnvString(virCommandPtr cmd, const char *str) { char *env; - if (!cmd || cmd->has_error) + if (virCommandHasError(cmd)) return; env = g_strdup(str); @@ -1421,7 +1442,7 @@ void virCommandAddEnvPass(virCommandPtr cmd, const char *name) { const char *value; - if (!cmd || cmd->has_error) + if (virCommandHasError(cmd)) return; value = getenv(name); @@ -1440,7 +1461,7 @@ virCommandAddEnvPass(virCommandPtr cmd, const char *name) void virCommandAddEnvPassCommon(virCommandPtr cmd) { - if (!cmd || cmd->has_error) + if (virCommandHasError(cmd)) return; ignore_value(VIR_RESIZE_N(cmd->env, cmd->maxenv, cmd->nenv, 9)); @@ -1460,7 +1481,7 @@ virCommandAddEnvPassCommon(virCommandPtr cmd) void virCommandAddEnvXDG(virCommandPtr cmd, const char *baseDir) { - if (!cmd || cmd->has_error) + if (virCommandHasError(cmd)) return; ignore_value(VIR_RESIZE_N(cmd->env, cmd->maxenv, cmd->nenv, 3)); @@ -1484,7 +1505,7 @@ virCommandAddEnvXDG(virCommandPtr cmd, const char *baseDir) void virCommandAddArg(virCommandPtr cmd, const char *val) { - if (!cmd || cmd->has_error) + if (virCommandHasError(cmd)) return; if (val == NULL) { @@ -1512,7 +1533,7 @@ virCommandAddArgBuffer(virCommandPtr cmd, virBufferPtr buf) { g_autofree char *str = virBufferContentAndReset(buf); - if (!cmd || cmd->has_error) + if (virCommandHasError(cmd)) return; if (!str) @@ -1540,7 +1561,7 @@ virCommandAddArgFormat(virCommandPtr cmd, const char *format, ...) char *arg; va_list list; - if (!cmd || cmd->has_error) + if (virCommandHasError(cmd)) return; va_start(list, format); @@ -1583,7 +1604,7 @@ virCommandAddArgSet(virCommandPtr cmd, const char *const*vals) { int narg = 0; - if (!cmd || cmd->has_error) + if (virCommandHasError(cmd)) return; if (vals[0] == NULL) { @@ -1619,7 +1640,7 @@ virCommandAddArgList(virCommandPtr cmd, ...) va_list list; int narg = 0; - if (!cmd || cmd->has_error) + if (virCommandHasError(cmd)) return; va_start(list, cmd); @@ -1653,7 +1674,7 @@ virCommandAddArgList(virCommandPtr cmd, ...) void virCommandSetWorkingDirectory(virCommandPtr cmd, const char *pwd) { - if (!cmd || cmd->has_error) + if (virCommandHasError(cmd)) return; if (cmd->pwd) { @@ -1701,7 +1722,7 @@ virCommandSetSendBuffer(virCommandPtr cmd, { size_t i; - if (!cmd || cmd->has_error) + if (virCommandHasError(cmd)) return -1; if (fcntl(fd, F_SETFL, O_NONBLOCK) < 0) { @@ -1795,7 +1816,7 @@ virCommandSendBuffersHandlePoll(virCommandPtr cmd, void virCommandSetInputBuffer(virCommandPtr cmd, const char *inbuf) { - if (!cmd || cmd->has_error) + if (virCommandHasError(cmd)) return; if (cmd->infd != -1 || cmd->inbuf) { @@ -1825,7 +1846,7 @@ void virCommandSetOutputBuffer(virCommandPtr cmd, char **outbuf) { *outbuf = NULL; - if (!cmd || cmd->has_error) + if (virCommandHasError(cmd)) return; if (cmd->outfdptr) { @@ -1859,7 +1880,7 @@ void virCommandSetErrorBuffer(virCommandPtr cmd, char **errbuf) { *errbuf = NULL; - if (!cmd || cmd->has_error) + if (virCommandHasError(cmd)) return; if (cmd->errfdptr) { @@ -1883,7 +1904,7 @@ virCommandSetErrorBuffer(virCommandPtr cmd, char **errbuf) void virCommandSetInputFD(virCommandPtr cmd, int infd) { - if (!cmd || cmd->has_error) + if (virCommandHasError(cmd)) return; if (cmd->infd != -1 || cmd->inbuf) { @@ -1913,7 +1934,7 @@ virCommandSetInputFD(virCommandPtr cmd, int infd) void virCommandSetOutputFD(virCommandPtr cmd, int *outfd) { - if (!cmd || cmd->has_error) + if (virCommandHasError(cmd)) return; if (cmd->outfdptr) { @@ -1939,7 +1960,7 @@ virCommandSetOutputFD(virCommandPtr cmd, int *outfd) void virCommandSetErrorFD(virCommandPtr cmd, int *errfd) { - if (!cmd || cmd->has_error) + if (virCommandHasError(cmd)) return; if (cmd->errfdptr) { @@ -1968,7 +1989,7 @@ virCommandSetErrorFD(virCommandPtr cmd, int *errfd) void virCommandSetPreExecHook(virCommandPtr cmd, virExecHook hook, void *opaque) { - if (!cmd || cmd->has_error) + if (virCommandHasError(cmd)) return; if (cmd->hook) { @@ -1999,7 +2020,7 @@ virCommandWriteArgLog(virCommandPtr cmd, int logfd) /* Any errors will be reported later by virCommandRun, which means * no command will be run, so there is nothing to log. */ - if (!cmd || cmd->has_error) + if (virCommandHasError(cmd)) return; for (i = 0; i < cmd->nenv; i++) { @@ -2043,11 +2064,8 @@ virCommandToString(virCommandPtr cmd, bool linebreaks) /* Cannot assume virCommandRun will be called; so report the error * now. If virCommandRun is called, it will report the same error. */ - if (!cmd || cmd->has_error) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("invalid use of command API")); + if (virCommandRaiseError(cmd) < 0) return NULL; - } for (i = 0; i < cmd->nenv; i++) { /* In shell, a='b c' has a different meaning than 'a=b c', so @@ -2092,11 +2110,8 @@ virCommandGetArgList(virCommandPtr cmd, { size_t i; - if (cmd->has_error) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("invalid use of command API")); + if (virCommandRaiseError(cmd) < 0) return -1; - } *args = g_new0(char *, cmd->nargs); *nargs = cmd->nargs - 1; @@ -2279,11 +2294,8 @@ virCommandProcessIO(virCommandPtr cmd) */ int virCommandExec(virCommandPtr cmd, gid_t *groups, int ngroups) { - if (!cmd || cmd->has_error) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("invalid use of command API")); + if (virCommandRaiseError(cmd) < 0) return -1; - } if (virExecCommon(cmd, groups, ngroups) < 0) return -1; @@ -2324,11 +2336,8 @@ virCommandRun(virCommandPtr cmd, int *exitstatus) char *str; int tmpfd; - if (!cmd || cmd->has_error) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("invalid use of command API")); + if (virCommandRaiseError(cmd) < 0) return -1; - } /* Avoid deadlock, by requiring that any open fd not under our * control must be visiting a regular file, or that we are @@ -2471,11 +2480,8 @@ virCommandRunAsync(virCommandPtr cmd, pid_t *pid) bool synchronous = false; int infd[2] = {-1, -1}; - if (!cmd || cmd->has_error) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("invalid use of command API")); + if (virCommandRaiseError(cmd) < 0) return -1; - } synchronous = cmd->flags & VIR_EXEC_RUN_SYNC; cmd->flags &= ~VIR_EXEC_RUN_SYNC; @@ -2620,11 +2626,8 @@ virCommandWait(virCommandPtr cmd, int *exitstatus) int ret; int status = 0; - if (!cmd || cmd->has_error) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("invalid use of command API")); + if (virCommandRaiseError(cmd) < 0) return -1; - } if (dryRunBuffer || dryRunCallback) { VIR_DEBUG("Dry run requested, returning status %d", @@ -2717,7 +2720,7 @@ virCommandAbort(virCommandPtr cmd) */ void virCommandRequireHandshake(virCommandPtr cmd) { - if (!cmd || cmd->has_error) + if (virCommandHasError(cmd)) return; if (cmd->handshake) { @@ -2760,7 +2763,10 @@ int virCommandHandshakeWait(virCommandPtr cmd) char c; int rv; - if (!cmd || cmd->has_error || !cmd->handshake) { + if (virCommandRaiseError(cmd) < 0) + return -1; + + if (!cmd->handshake) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("invalid use of command API")); return -1; @@ -2818,7 +2824,10 @@ int virCommandHandshakeNotify(virCommandPtr cmd) { char c = '1'; - if (!cmd || cmd->has_error || !cmd->handshake) { + if (virCommandRaiseError(cmd) < 0) + return -1; + + if (!cmd->handshake) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("invalid use of command API")); return -1; @@ -2846,7 +2855,7 @@ virCommandSetSendBuffer(virCommandPtr cmd, unsigned char *buffer G_GNUC_UNUSED, size_t buflen G_GNUC_UNUSED) { - if (!cmd || cmd->has_error) + if (virCommandHasError(cmd)) return -1; cmd->has_error = ENOTSUP; @@ -2903,7 +2912,7 @@ virCommandAbort(virCommandPtr cmd G_GNUC_UNUSED) void virCommandRequireHandshake(virCommandPtr cmd) { - if (!cmd || cmd->has_error) + if (virCommandHasError(cmd)) return; cmd->has_error = ENOSYS; @@ -3030,7 +3039,7 @@ virCommandFree(virCommandPtr cmd) void virCommandDoAsyncIO(virCommandPtr cmd) { - if (!cmd || cmd->has_error) + if (virCommandHasError(cmd)) return; cmd->flags |= VIR_EXEC_ASYNC_IO | VIR_EXEC_NONBLOCK; -- 2.29.2
On a Tuesday in 2021, Peter Krempa wrote:
Extract the check and reporting of error from the individual virCommand APIs into a separate helper. This will aid future refactors.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/util/vircommand.c | 143 ++++++++++++++++++++++-------------------- 1 file changed, 76 insertions(+), 67 deletions(-)
@@ -2760,7 +2763,10 @@ int virCommandHandshakeWait(virCommandPtr cmd) char c; int rv;
- if (!cmd || cmd->has_error || !cmd->handshake) { + if (virCommandRaiseError(cmd) < 0) + return -1; + + if (!cmd->handshake) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("invalid use of command API")); return -1; @@ -2818,7 +2824,10 @@ int virCommandHandshakeNotify(virCommandPtr cmd) { char c = '1';
- if (!cmd || cmd->has_error || !cmd->handshake) { + if (virCommandRaiseError(cmd) < 0) + return -1; +
From the name of the function, I'd expect it to raise the error unconditionally. Cache invalidation and naming are hard. CheckError might be a misleading name too [0]. MaybeRaiseError? RaiseErrorIfNeeded? Would returning bool make more sense here? [0] commit 5c63b50a8b9f18b9ab18753cb679065cd31895fb conf: rename virDomainCheckVirtioOptions Also, in both cases the same error message from virCommandRaiseError is repeated if (!cmd->handshake). Consider void virCommandRaiseError and: if (virCommandHasError || !cmd->handshake) { virCommandRaiseError(); return -1; }
+ if (!cmd->handshake) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("invalid use of command API"));
return -1;
If you consider any of the points above: Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
The function can't fail nowadays. Remove the return value and adjust the only caller which ensures that @cmd is non-NULL and @fd is positive. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/util/vircommand.c | 22 +++------------------- 1 file changed, 3 insertions(+), 19 deletions(-) diff --git a/src/util/vircommand.c b/src/util/vircommand.c index 2f6635671d..579c77fd9f 100644 --- a/src/util/vircommand.c +++ b/src/util/vircommand.c @@ -216,27 +216,19 @@ virCommandFDIsSet(virCommandPtr cmd, * This is practically generalized implementation * of FD_SET() as we do not want to be limited * by FD_SETSIZE. - * - * Returns: 0 on success, - * -1 on usage error, */ -static int +static void virCommandFDSet(virCommandPtr cmd, int fd, unsigned int flags) { - if (!cmd || fd < 0) - return -1; - if (virCommandFDIsSet(cmd, fd)) - return 0; + return; ignore_value(VIR_EXPAND_N(cmd->passfd, cmd->npassfd, 1)); cmd->passfd[cmd->npassfd - 1].fd = fd; cmd->passfd[cmd->npassfd - 1].flags = flags; - - return 0; } #ifndef WIN32 @@ -1035,8 +1027,6 @@ virCommandNewVAList(const char *binary, va_list list) void virCommandPassFDIndex(virCommandPtr cmd, int fd, unsigned int flags, size_t *idx) { - int ret = 0; - if (!cmd) { VIR_COMMAND_MAYBE_CLOSE_FD(fd, flags); return; @@ -1050,13 +1040,7 @@ virCommandPassFDIndex(virCommandPtr cmd, int fd, unsigned int flags, size_t *idx return; } - if ((ret = virCommandFDSet(cmd, fd, flags)) != 0) { - if (!cmd->has_error) - cmd->has_error = ret; - VIR_DEBUG("cannot preserve %d", fd); - VIR_COMMAND_MAYBE_CLOSE_FD(fd, flags); - return; - } + virCommandFDSet(cmd, fd, flags); if (idx) *idx = cmd->npassfd - 1; -- 2.29.2
On a Tuesday in 2021, Peter Krempa wrote:
The function can't fail nowadays. Remove the return value and adjust the only caller which ensures that @cmd is non-NULL and @fd is positive.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/util/vircommand.c | 22 +++------------------- 1 file changed, 3 insertions(+), 19 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
The function is used to automatically feed a buffer into a pipe which can be used by the command to read contents of the buffer. Rather than passing in a pipe, let's create the pipe inside virCommandSetSendBuffer and directly associate the reader end with the command. This way the ownership of both ends of the pipe will end up with the virCommand right away reducing the need of cleanup in callers. The returned value then can be used just to format the appropriate arguments without worrying about cleanup or failure. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_tpm.c | 50 +++++++++---------------------------------- src/util/vircommand.c | 39 ++++++++++++++++++++++----------- src/util/vircommand.h | 6 +++--- tests/commandtest.c | 32 ++++++--------------------- 4 files changed, 46 insertions(+), 81 deletions(-) diff --git a/src/qemu/qemu_tpm.c b/src/qemu/qemu_tpm.c index d994816484..d8c518fc73 100644 --- a/src/qemu/qemu_tpm.c +++ b/src/qemu/qemu_tpm.c @@ -353,14 +353,14 @@ qemuTPMEmulatorPrepareHost(virDomainTPMDefPtr tpm, * This function reads the passphrase and writes it into the * write-end of a pipe so that the read-end of the pipe can be * passed to the emulator for reading the passphrase from. + * + * Note that the returned FD is owned by @cmd. */ static int qemuTPMSetupEncryption(const unsigned char *secretuuid, virCommandPtr cmd) { - int ret = -1; - int pipefd[2] = { -1, -1 }; - virConnectPtr conn; + g_autoptr(virConnect) conn = NULL; g_autofree uint8_t *secret = NULL; size_t secret_len; virSecretLookupTypeDef seclookupdef = { @@ -375,27 +375,9 @@ qemuTPMSetupEncryption(const unsigned char *secretuuid, if (virSecretGetSecretString(conn, &seclookupdef, VIR_SECRET_USAGE_TYPE_VTPM, &secret, &secret_len) < 0) - goto error; - - if (virPipe(pipefd) < 0) - goto error; - - if (virCommandSetSendBuffer(cmd, pipefd[1], secret, secret_len) < 0) - goto error; - - secret = NULL; - ret = pipefd[0]; - - cleanup: - virObjectUnref(conn); - - return ret; - - error: - VIR_FORCE_CLOSE(pipefd[1]); - VIR_FORCE_CLOSE(pipefd[0]); + return -1; - goto cleanup; + return virCommandSetSendBuffer(cmd, g_steal_pointer(&secret), secret_len); } /* @@ -549,8 +531,8 @@ qemuTPMEmulatorBuildCommand(virDomainTPMDefPtr tpm, bool created = false; g_autofree char *pidfile = NULL; g_autofree char *swtpm = virTPMGetSwtpm(); - VIR_AUTOCLOSE pwdfile_fd = -1; - VIR_AUTOCLOSE migpwdfile_fd = -1; + int pwdfile_fd = -1; + int migpwdfile_fd = -1; const unsigned char *secretuuid = NULL; if (!swtpm) @@ -618,25 +600,13 @@ qemuTPMEmulatorBuildCommand(virDomainTPMDefPtr tpm, } pwdfile_fd = qemuTPMSetupEncryption(tpm->data.emulator.secretuuid, cmd); - if (pwdfile_fd) { - migpwdfile_fd = qemuTPMSetupEncryption(tpm->data.emulator.secretuuid, - cmd); - } - - if (pwdfile_fd < 0 || migpwdfile_fd < 0) - goto error; + migpwdfile_fd = qemuTPMSetupEncryption(tpm->data.emulator.secretuuid, cmd); virCommandAddArg(cmd, "--key"); - virCommandAddArgFormat(cmd, "pwdfd=%d,mode=aes-256-cbc", - pwdfile_fd); - virCommandPassFD(cmd, pwdfile_fd, VIR_COMMAND_PASS_FD_CLOSE_PARENT); - pwdfile_fd = -1; + virCommandAddArgFormat(cmd, "pwdfd=%d,mode=aes-256-cbc", pwdfile_fd); virCommandAddArg(cmd, "--migration-key"); - virCommandAddArgFormat(cmd, "pwdfd=%d,mode=aes-256-cbc", - migpwdfile_fd); - virCommandPassFD(cmd, migpwdfile_fd, VIR_COMMAND_PASS_FD_CLOSE_PARENT); - migpwdfile_fd = -1; + virCommandAddArgFormat(cmd, "pwdfd=%d,mode=aes-256-cbc", migpwdfile_fd); } return g_steal_pointer(&cmd); diff --git a/src/util/vircommand.c b/src/util/vircommand.c index 579c77fd9f..04964b730b 100644 --- a/src/util/vircommand.c +++ b/src/util/vircommand.c @@ -1694,39 +1694,55 @@ virCommandFreeSendBuffers(virCommandPtr cmd) /** * virCommandSetSendBuffer * @cmd: the command to modify + * @buffer: buffer to pass to the filedescriptror + * @buflen: length of @buffer * - * Pass a buffer to virCommand that will be written into the - * given file descriptor. The buffer will be freed automatically - * and the file descriptor closed. + * Registers @buffer as an input buffer for @cmd which will be accessible via + * the returned file descriptor. The returned file descriptor is already + * registered to be passed to @cmd, so callers must use it only to format the + * appropriate argument of @cmd. + * + * @buffer is always stolen regardless of the return value. This function + * doesn't raise a libvirt error, but rather propagates the error via virCommand. + * Thus callers don't need to take a special action if -1 is returned. */ int virCommandSetSendBuffer(virCommandPtr cmd, - int fd, - unsigned char *buffer, size_t buflen) + unsigned char *buffer, + size_t buflen) { + g_autofree unsigned char *localbuf = g_steal_pointer(&buffer); + int pipefd[2] = { -1, -1 }; size_t i; if (virCommandHasError(cmd)) return -1; - if (fcntl(fd, F_SETFL, O_NONBLOCK) < 0) { - virReportSystemError(errno, "%s", - _("fcntl failed to set O_NONBLOCK")); + if (virPipeQuiet(pipefd) < 0) { + cmd->has_error = errno; + return -1; + } + + if (fcntl(pipefd[1], F_SETFL, O_NONBLOCK) < 0) { cmd->has_error = errno; + VIR_FORCE_CLOSE(pipefd[0]); + VIR_FORCE_CLOSE(pipefd[1]); return -1; } i = virCommandGetNumSendBuffers(cmd); ignore_value(VIR_REALLOC_N(cmd->sendBuffers, i + 1)); - cmd->sendBuffers[i].fd = fd; - cmd->sendBuffers[i].buffer = buffer; + cmd->sendBuffers[i].fd = pipefd[1]; + cmd->sendBuffers[i].buffer = g_steal_pointer(&localbuf); cmd->sendBuffers[i].buflen = buflen; cmd->sendBuffers[i].offset = 0; cmd->numSendBuffers++; - return 0; + virCommandPassFD(cmd, pipefd[0], VIR_COMMAND_PASS_FD_CLOSE_PARENT); + + return pipefd[0]; } @@ -2835,7 +2851,6 @@ int virCommandHandshakeNotify(virCommandPtr cmd) #else /* WIN32 */ int virCommandSetSendBuffer(virCommandPtr cmd, - int fd G_GNUC_UNUSED, unsigned char *buffer G_GNUC_UNUSED, size_t buflen G_GNUC_UNUSED) { diff --git a/src/util/vircommand.h b/src/util/vircommand.h index 9fb625ec4b..a00f30f51f 100644 --- a/src/util/vircommand.h +++ b/src/util/vircommand.h @@ -141,9 +141,9 @@ void virCommandSetWorkingDirectory(virCommandPtr cmd, const char *pwd) ATTRIBUTE_NONNULL(2); int virCommandSetSendBuffer(virCommandPtr cmd, - int fd, - unsigned char *buffer, size_t buflen) - ATTRIBUTE_NONNULL(3); + unsigned char *buffer, + size_t buflen) + ATTRIBUTE_NONNULL(2); void virCommandSetInputBuffer(virCommandPtr cmd, const char *inbuf) ATTRIBUTE_NONNULL(2); diff --git a/tests/commandtest.c b/tests/commandtest.c index df86725f0e..524c959eba 100644 --- a/tests/commandtest.c +++ b/tests/commandtest.c @@ -1039,8 +1039,8 @@ static int test26(const void *unused G_GNUC_UNUSED) static int test27(const void *unused G_GNUC_UNUSED) { g_autoptr(virCommand) cmd = virCommandNew(abs_builddir "/commandhelper"); - int pipe1[2]; - int pipe2[2]; + int buf1fd; + int buf2fd; int ret = -1; size_t buflen = 1024 * 128; g_autofree char *buffer0 = NULL; @@ -1078,30 +1078,14 @@ static int test27(const void *unused G_GNUC_UNUSED) errexpect = g_strdup_printf(TEST27_ERREXPECT_TEMP, buffer0, buffer1, buffer2); - if (virPipeQuiet(pipe1) < 0 || virPipeQuiet(pipe2) < 0) { - printf("Could not create pipe: %s\n", g_strerror(errno)); - goto cleanup; - } - - if (virCommandSetSendBuffer(cmd, pipe1[1], - (unsigned char *)buffer1, buflen - 1) < 0 || - virCommandSetSendBuffer(cmd, pipe2[1], - (unsigned char *)buffer2, buflen - 1) < 0) { - printf("Could not set send buffers\n"); - goto cleanup; - } - pipe1[1] = -1; - pipe2[1] = -1; - buffer1 = NULL; - buffer2 = NULL; + buf1fd = virCommandSetSendBuffer(cmd, (unsigned char *) g_steal_pointer(&buffer1), buflen - 1); + buf2fd = virCommandSetSendBuffer(cmd, (unsigned char *) g_steal_pointer(&buffer2), buflen - 1); virCommandAddArg(cmd, "--readfd"); - virCommandAddArgFormat(cmd, "%d", pipe1[0]); - virCommandPassFD(cmd, pipe1[0], 0); + virCommandAddArgFormat(cmd, "%d", buf1fd); virCommandAddArg(cmd, "--readfd"); - virCommandAddArgFormat(cmd, "%d", pipe2[0]); - virCommandPassFD(cmd, pipe2[0], 0); + virCommandAddArgFormat(cmd, "%d", buf2fd); virCommandSetInputBuffer(cmd, buffer0); virCommandSetOutputBuffer(cmd, &outactual); @@ -1130,10 +1114,6 @@ static int test27(const void *unused G_GNUC_UNUSED) ret = 0; cleanup: - VIR_FORCE_CLOSE(pipe1[0]); - VIR_FORCE_CLOSE(pipe2[0]); - VIR_FORCE_CLOSE(pipe1[1]); - VIR_FORCE_CLOSE(pipe2[1]); return ret; } -- 2.29.2
On a Tuesday in 2021, Peter Krempa wrote:
The function is used to automatically feed a buffer into a pipe which can be used by the command to read contents of the buffer.
Rather than passing in a pipe, let's create the pipe inside virCommandSetSendBuffer and directly associate the reader end with the command. This way the ownership of both ends of the pipe will end up with the virCommand right away reducing the need of cleanup in callers.
The returned value then can be used just to format the appropriate arguments without worrying about cleanup or failure.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_tpm.c | 50 +++++++++---------------------------------- src/util/vircommand.c | 39 ++++++++++++++++++++++----------- src/util/vircommand.h | 6 +++--- tests/commandtest.c | 32 ++++++--------------------- 4 files changed, 46 insertions(+), 81 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- tests/commandtest.c | 17 ++++++----------- 1 file changed, 6 insertions(+), 11 deletions(-) diff --git a/tests/commandtest.c b/tests/commandtest.c index 524c959eba..aaf391935c 100644 --- a/tests/commandtest.c +++ b/tests/commandtest.c @@ -1041,7 +1041,6 @@ static int test27(const void *unused G_GNUC_UNUSED) g_autoptr(virCommand) cmd = virCommandNew(abs_builddir "/commandhelper"); int buf1fd; int buf2fd; - int ret = -1; size_t buflen = 1024 * 128; g_autofree char *buffer0 = NULL; g_autofree char *buffer1 = NULL; @@ -1093,29 +1092,25 @@ static int test27(const void *unused G_GNUC_UNUSED) if (virCommandRun(cmd, NULL) < 0) { printf("Cannot run child %s\n", virGetLastErrorMessage()); - goto cleanup; + return -1; } if (!outactual || !erractual) - goto cleanup; + return -1; if (STRNEQ(outactual, outexpect)) { virTestDifference(stderr, outexpect, outactual); - goto cleanup; + return -1; } if (STRNEQ(erractual, errexpect)) { virTestDifference(stderr, errexpect, erractual); - goto cleanup; + return -1; } if (checkoutput("test27") < 0) - goto cleanup; - - ret = 0; - - cleanup: + return -1; - return ret; + return 0; } -- 2.29.2
On a Tuesday in 2021, Peter Krempa wrote:
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- tests/commandtest.c | 17 ++++++----------- 1 file changed, 6 insertions(+), 11 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
participants (2)
-
Ján Tomko -
Peter Krempa