Re: [libvirt] [PATCHv2 3/8] Introduce new APIs for spawning processes
On 12/01/2010 02:42 PM, Matthias Bolte wrote:
>> +++ b/cfg.mk
>> @@ -77,6 +77,7 @@ useless_free_options =\
>> --name=virCapabilitiesFreeHostNUMACell \
>> --name=virCapabilitiesFreeMachines \
>> --name=virCgroupFree \
>> + --name=virCommandFree\
>> --name=virConfFreeList \
>> --name=virConfFreeValue \
>> --name=virDomainChrDefFree \
>
> You should also add virCommandError to the msg_gen_function list.
Good catch, and done.
>> +void
>> +virCommandSetOutputBuffer(virCommandPtr cmd, char **outbuf)
>> +{
>> +if (!cmd || cmd->has_error)
>> +return;
>> +
>> +if (cmd->outfd != -1) {
>
> To detect double assignment properly you need to check for this I think:
>
> if (cmd->outfd != -1 || cmd->outfdptr || cmd->outbuf) {
Almost. There are really only two functions that affect stdout settings
before a command (set fd assigns outfdptr, set buffer assigns outbuf and
outfdptr), so checking cmd->outfdptr for NULL is sufficient to check for
no duplicate call.
>> +void
>> +virCommandSetInputFD(virCommandPtr cmd, int infd)
>> +{
>> +if (!cmd || cmd->has_error)
>> +return;
>> +
>> +if (infd < 0 || cmd->inbuf) {
>
> I think you meant to check here for this instead:
>
> if (cmd->infd != -1 || cmd->inbuf) {
Hmm; actually, there's two issues. One of validating that input is set
at most once (cmd->infd != -1 || cmd->inbuf), and one of validating that
the incoming argument is valid (infd < 0), worth two separate messages.
Thanks for forcing me to think about this more.
>> +void
>> +virCommandWriteArgLog(virCommandPtr cmd, int logfd)
>> +{
>> +int ioError = 0;
>> +size_t i;
>> +
>> +/* 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)
>> +return;
>> +
>> +for (i = 0 ; i < cmd->nenv ; i++) {
>> +if (safewrite(logfd, cmd->env[i], strlen(cmd->env[i])) < 0)
>> +ioError = errno;
>> +if (safewrite(logfd, " ", 1) < 0)
>> +ioError = errno;
>> +}
>> +for (i = 0 ; i < cmd->nargs ; i++) {
>> +if (safewrite(logfd, cmd->args[i], strlen(cmd->args[i])) < 0)
>> +ioError = errno;
>> +if (safewrite(logfd, i == cmd->nargs - 1 ? "\n" : " ", 1) < 0)
>> +ioError = errno;
>> +}
>
> As written this will only save the last occurred error in ioError. Is
> this intended? Looks like a best effort approach, try to write as much
> as possible ignoring errors.
Well, the function has no return value, so yes, that's the best we can
do - log as much as possible, and issue a VIR_WARN if we didn't log
everything. But I couldn't seem to justify returning an error and
stopping the log at the first error - how do you log that you had an
error logging, when logging is orthogonal to running the command in the
first place?
>
> In virCommandProcessIO you say that cmd->{out|err}fd is a valid FD
> (created in virExecWithHook called by virCommandRunAsync) when
> cmd->{out|err}buf is not NULL. As far as I can tell from the code that
> is true when the caller had requested to capture stdout and stderr.
> But in case that caller didn't do this then you setup buffers to
> capture stdout and stderr for logging. In that case
> virCommandProcessIO will be called with cmd->{out|err}buf being
> non-NULL but cmd->{out|err}fd being invalid as you explicitly set them
> to -1 in the two if blocks before the call to virCommandProcessIO.
Yep, that needed reordering. If virCommandRun detects that nothing set
output, then outfd needs to be set prior to virCommandRunAsync so as to
force the creation of a pipe rather than assigning fds to /dev/null.
>> +
>> +#ifndef __linux__
>
> What's Linux specific in this test? Shouldn't the command API and this
> test be working on all systems that support fork/exec?
It should really be #if !HAVE_FORK (aka #ifdef WIN32).
I'm testing the impacts of squashing this in...
diff --git i/cfg.mk w/cfg.mk
index 8a8da18..29de9d9 100644
--- i/cfg.mk
+++ w/cfg.mk
@@ -369,9 +369,9 @@ msg_gen_function += umlReportError
msg_gen_function += vah_error
msg_gen_function += vah_warning
msg_gen_function += vboxError
+msg_gen_function += virCommandError
msg_gen_function += virConfError
msg_gen_function += virDomainReportError
-msg_gen_function += virSecurityReportError
msg_gen_function += virHashError
msg_gen_function += virLibConnError
msg_gen_function += virLibDomainError
@@ -380,6 +380,7 @@ msg_gen_function += virNodeDeviceReportError
msg_gen_function += virRaiseError
msg_gen_function += virReportErrorHelper
msg_gen_function += virReportSystemError
+msg_gen_function += virSecurityReportError
msg_gen_function += virSexprError
msg_gen_function += virStorageReportError
msg_gen_function += virXMLError
Re: [libvirt] [PATCHv2 3/8] Introduce new APIs for spawning processes
2010/12/2 Daniel P. Berrange :
> On Wed, Dec 01, 2010 at 10:42:40PM +0100, Matthias Bolte wrote:
>> 2010/11/24 Eric Blake :
>> > +/*
>> > + * Manage input and output to the child process.
>> > + */
>> > +static int
>> > +virCommandProcessIO(virCommandPtr cmd)
>> > +{
>> > + int infd = -1, outfd = -1, errfd = -1;
>> > + size_t inlen = 0, outlen = 0, errlen = 0;
>> > + size_t inoff = 0;
>> > +
>> > + /* With an input buffer, feed data to child
>> > + * via pipe */
>> > + if (cmd->inbuf) {
>> > + inlen = strlen(cmd->inbuf);
>> > + infd = cmd->inpipe;
>> > + }
>> > +
>> > + /* With out/err buffer, the outfd/errfd
>> > + * have been filled with an FD for us */
>> > + if (cmd->outbuf)
>> > + outfd = cmd->outfd;
>> > + if (cmd->errbuf)
>> > + errfd = cmd->errfd;
>> > +
>>
>> > +/*
>> > + * Run the command and wait for completion.
>> > + * Returns -1 on any error executing the
>> > + * command. Returns 0 if the command executed,
>> > + * with the exit status set
>> > + */
>> > +int
>> > +virCommandRun(virCommandPtr cmd, int *exitstatus)
>> > +{
>> > + int ret = 0;
>> > + char *outbuf = NULL;
>> > + char *errbuf = NULL;
>> > + int infd[2];
>> > +
>> > + if (!cmd || cmd->has_error == -1) {
>> > + virCommandError(VIR_ERR_INTERNAL_ERROR, "%s",
>> > + _("invalid use of command API"));
>> > + return -1;
>> > + }
>> > + if (cmd->has_error == ENOMEM) {
>> > + virReportOOMError();
>> > + return -1;
>> > + }
>> > +
>> > + /* If we have an input buffer, we need
>> > + * a pipe to feed the data to the child */
>> > + if (cmd->inbuf) {
>> > + if (pipe(infd) < 0) {
>> > + virReportSystemError(errno, "%s",
>> > + _("unable to open pipe"));
>> > + return -1;
>> > + }
>> > + cmd->infd = infd[0];
>> > + cmd->inpipe = infd[1];
>> > + }
>> > +
>> > + if (virCommandRunAsync(cmd, NULL) < 0) {
>> > + if (cmd->inbuf) {
>> > + int tmpfd = infd[0];
>> > + if (VIR_CLOSE(infd[0]) < 0)
>> > + VIR_DEBUG("ignoring failed close on fd %d", tmpfd);
>> > + tmpfd = infd[1];
>> > + if (VIR_CLOSE(infd[1]) < 0)
>> > + VIR_DEBUG("ignoring failed close on fd %d", tmpfd);
>> > + }
>> > + return -1;
>> > + }
>> > +
>> > + /* If caller hasn't requested capture of
>> > + * stdout/err, then capture it ourselves
>> > + * so we can log it
>> > + */
>> > + if (!cmd->outbuf &&
>> > + !cmd->outfdptr) {
>> > + cmd->outfd = -1;
>> > + cmd->outfdptr = &cmd->outfd;
>> > + cmd->outbuf = &outbuf;
>> > + }
>> > + if (!cmd->errbuf &&
>> > + !cmd->errfdptr) {
>> > + cmd->errfd = -1;
>> > + cmd->errfdptr = &cmd->errfd;
>> > + cmd->errbuf = &errbuf;
>> > + }
>> > +
>> > + if (cmd->inbuf || cmd->outbuf || cmd->errbuf)
>> > + ret = virCommandProcessIO(cmd);
>>
>> In virCommandProcessIO you say that cmd->{out|err}fd is a valid FD
>> (created in virExecWithHook called by virCommandRunAsync) when
>> cmd->{out|err}buf is not NULL. As far as I can tell from the code that
>> is true when the caller had requested to capture stdout and stderr.
>> But in case that caller didn't do this then you setup buffers to
>> capture stdout and stderr for logging. In that case
>> virCommandProcessIO will be called with cmd->{out|err}buf being
>> non-NULL but cmd->{out|err}fd being invalid as you explicitly set them
>> to -1 in the two if blocks before the call to virCommandProcessIO.
>
> Those two blocks setting errfd/outfd to -1 are not executed
> when outbuf/errbuf are non-NULL, so errfd/outfd are still
> pointing to the pipe() FDs when virCommandProcesIO is run.
>
Yes, that's true, but that's not the case I'm talking about.
The case I'm talking about is when the user of the command API didn't
set the cmd->outfdptr nor the cmd->outbuf, so both are NULL. In that
case when the chain virCommandRunAsync -> virExecWithHook -> __virExec
is called __virExec will bind stdout of the child to /dev/null instead
of a pipe.
So when changing cmd->outfdptr and cmd->outbuf to non-NULL values
after the call to virCommandRunAsync this will not result in having a
valid FD assigned to cmd->outfd before the call to
virCommandProcessIO. Therefore, virCommandProcessIO won't capture
stdout and stderr for logging purpose.
I still think that the two if blocks should be moved in front of
virCommandRunAsync to achieve the described behavior, in the case that
the command API user didn't request to capture stdout/stderr.
Matthias
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCHv2 3/8] Introduce new APIs for spawning processes
On Wed, Dec 01, 2010 at 10:42:40PM +0100, Matthias Bolte wrote:
> 2010/11/24 Eric Blake :
> > +/*
> > + * Manage input and output to the child process.
> > + */
> > +static int
> > +virCommandProcessIO(virCommandPtr cmd)
> > +{
> > + int infd = -1, outfd = -1, errfd = -1;
> > + size_t inlen = 0, outlen = 0, errlen = 0;
> > + size_t inoff = 0;
> > +
> > + /* With an input buffer, feed data to child
> > + * via pipe */
> > + if (cmd->inbuf) {
> > + inlen = strlen(cmd->inbuf);
> > + infd = cmd->inpipe;
> > + }
> > +
> > + /* With out/err buffer, the outfd/errfd
> > + * have been filled with an FD for us */
> > + if (cmd->outbuf)
> > + outfd = cmd->outfd;
> > + if (cmd->errbuf)
> > + errfd = cmd->errfd;
> > +
>
> > +/*
> > + * Run the command and wait for completion.
> > + * Returns -1 on any error executing the
> > + * command. Returns 0 if the command executed,
> > + * with the exit status set
> > + */
> > +int
> > +virCommandRun(virCommandPtr cmd, int *exitstatus)
> > +{
> > + int ret = 0;
> > + char *outbuf = NULL;
> > + char *errbuf = NULL;
> > + int infd[2];
> > +
> > + if (!cmd || cmd->has_error == -1) {
> > + virCommandError(VIR_ERR_INTERNAL_ERROR, "%s",
> > + _("invalid use of command API"));
> > + return -1;
> > + }
> > + if (cmd->has_error == ENOMEM) {
> > + virReportOOMError();
> > + return -1;
> > + }
> > +
> > + /* If we have an input buffer, we need
> > + * a pipe to feed the data to the child */
> > + if (cmd->inbuf) {
> > + if (pipe(infd) < 0) {
> > + virReportSystemError(errno, "%s",
> > + _("unable to open pipe"));
> > + return -1;
> > + }
> > + cmd->infd = infd[0];
> > + cmd->inpipe = infd[1];
> > + }
> > +
> > + if (virCommandRunAsync(cmd, NULL) < 0) {
> > + if (cmd->inbuf) {
> > + int tmpfd = infd[0];
> > + if (VIR_CLOSE(infd[0]) < 0)
> > + VIR_DEBUG("ignoring failed close on fd %d", tmpfd);
> > + tmpfd = infd[1];
> > + if (VIR_CLOSE(infd[1]) < 0)
> > + VIR_DEBUG("ignoring failed close on fd %d", tmpfd);
> > + }
> > + return -1;
> > + }
> > +
> > + /* If caller hasn't requested capture of
> > + * stdout/err, then capture it ourselves
> > + * so we can log it
> > + */
> > + if (!cmd->outbuf &&
> > + !cmd->outfdptr) {
> > + cmd->outfd = -1;
> > + cmd->outfdptr = &cmd->outfd;
> > + cmd->outbuf = &outbuf;
> > + }
> > + if (!cmd->errbuf &&
> > + !cmd->errfdptr) {
> > + cmd->errfd = -1;
> > + cmd->errfdptr = &cmd->errfd;
> > + cmd->errbuf = &errbuf;
> > + }
> > +
> > + if (cmd->inbuf || cmd->outbuf || cmd->errbuf)
> > + ret = virCommandProcessIO(cmd);
>
> In virCommandProcessIO you say that cmd->{out|err}fd is a valid FD
> (created in virExecWithHook called by virCommandRunAsync) when
> cmd->{out|err}buf is not NULL. As far as I can tell from the code that
> is true when the caller had requested to capture stdout and stderr.
> But in case that caller didn't do this then you setup buffers to
> capture stdout and stderr for logging. In that case
> virCommandProcessIO will be called with cmd->{out|err}buf being
> non-NULL but cmd->{out|err}fd being invalid as you explicitly set them
> to -1 in the two if blocks before the call to virCommandProcessIO.
Those two blocks setting errfd/outfd to -1 are not executed
when outbuf/errbuf are non-NULL, so errfd/outfd are still
pointing to the pipe() FDs when virCommandProcesIO is run.
> > diff --git a/tests/commandtest.c b/tests/commandtest.c
> > new file mode 100644
> > index 000..46d6ae5
> > --- /dev/null
> > +++ b/tests/commandtest.c
>
> > +
> > +#ifndef __linux__
>
> What's Linux specific in this test? Shouldn't the command API and this
> test be working on all systems that support fork/exec?
It should have been #ifndef WIN32 actually.
Daniel
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCHv2 3/8] Introduce new APIs for spawning processes
On Wed, Dec 01, 2010 at 04:39:25PM -0700, Eric Blake wrote: > On 11/23/2010 04:49 PM, Eric Blake wrote: > > From: Daniel P. Berrange > > > > This introduces a new set of APIs in src/util/command.h > > to use for invoking commands. This is intended to replace > > all current usage of virRun and virExec variants, with a > > more flexible and less error prone API. > > > > +/* > > + * Call after adding all arguments and environment settings, but before > > + * Run/RunAsync, to immediately output the environment and arguments of > > + * cmd to logfd. If virCommandRun cannot succeed (because of an > > + * out-of-memory condition while building cmd), nothing will be logged. > > + */ > > +void virCommandWriteArgLog(virCommandPtr cmd, > > + int logfd); > > + > > +/* > > + * Call after adding all arguments and environment settings, but before > > + * Run/RunAsync, to return a string representation of the environment and > > + * arguments of cmd. If virCommandRun cannot succeed (because of an > > + * out-of-memory condition while building cmd), NULL will be returned. > > + * Caller is responsible for freeing the resulting string. > > + */ > > +char *virCommandToString(virCommandPtr cmd) ATTRIBUTE_RETURN_CHECK; > > Bummer. Just realized that these functions should probably be modified > to take another parameter that controls whether the output should be > quoted for shell use. I don't think this is particularly important given the usage made of these API. It is just a nice to have addition, which shouldn't delay the patchset. Dnaiel -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCHv2 3/8] Introduce new APIs for spawning processes
On Tue, Nov 23, 2010 at 04:49:56PM -0700, Eric Blake wrote: > From: Daniel P. Berrange > > This introduces a new set of APIs in src/util/command.h > to use for invoking commands. This is intended to replace > all current usage of virRun and virExec variants, with a > more flexible and less error prone API. > > * src/util/command.c: New file. > * src/util/command.h: New header. > * src/Makefile.am (UTIL_SOURCES): Build it. > * src/libvirt_private.syms: Export symbols internally. > * tests/commandtest.c: New test. > * tests/Makefile.am (check_PROGRAMS): Run it. > * tests/commandhelper.c: Auxiliary program. > * tests/commanddata/test2.log - test15.log: New expected outputs. > * cfg.mk (useless_free_options): Add virCommandFree. > * po/POTFILES.in: New translation. > * .x-sc_avoid_write: Add exemption. > * tests/.gitignore: Ignore new built file. ACK Daniel -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCHv2 3/8] Introduce new APIs for spawning processes
On 11/23/2010 04:49 PM, Eric Blake wrote: > From: Daniel P. Berrange > > This introduces a new set of APIs in src/util/command.h > to use for invoking commands. This is intended to replace > all current usage of virRun and virExec variants, with a > more flexible and less error prone API. > > +/* > + * Call after adding all arguments and environment settings, but before > + * Run/RunAsync, to immediately output the environment and arguments of > + * cmd to logfd. If virCommandRun cannot succeed (because of an > + * out-of-memory condition while building cmd), nothing will be logged. > + */ > +void virCommandWriteArgLog(virCommandPtr cmd, > + int logfd); > + > +/* > + * Call after adding all arguments and environment settings, but before > + * Run/RunAsync, to return a string representation of the environment and > + * arguments of cmd. If virCommandRun cannot succeed (because of an > + * out-of-memory condition while building cmd), NULL will be returned. > + * Caller is responsible for freeing the resulting string. > + */ > +char *virCommandToString(virCommandPtr cmd) ATTRIBUTE_RETURN_CHECK; Bummer. Just realized that these functions should probably be modified to take another parameter that controls whether the output should be quoted for shell use. Basically, I just tested the recent addition of , and found out that it has a bug: it takes the host string (which may have spaces), and passes it to the qemu command line as: -smbios 'type=0,version="6FET82WW (3.12 )"' which means that the guest gets literal "" in the SMBIOS string, whereas the host did not have them, such that it is not a true host mirroring as intended. But in the /var/log/libvirt/qemu/...log, the argument only shows up as: -smbios type=0,version="6FET82WW (3.12 )" which, when pasted literally into a shell, does the right thing. Therefore, adding the ability to shell-quote the log will make it easier to log complex shell commands, where we really did intend to pass shell metacharacters via a single argument. I'll have to fold that in to my next revision. -- Eric Blake ebl...@redhat.com+1-801-349-2682 Libvirt virtualization library http://libvirt.org signature.asc Description: OpenPGP digital signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCHv2 3/8] Introduce new APIs for spawning processes
2010/11/24 Eric Blake :
> From: Daniel P. Berrange
>
> This introduces a new set of APIs in src/util/command.h
> to use for invoking commands. This is intended to replace
> all current usage of virRun and virExec variants, with a
> more flexible and less error prone API.
>
> * src/util/command.c: New file.
> * src/util/command.h: New header.
> * src/Makefile.am (UTIL_SOURCES): Build it.
> * src/libvirt_private.syms: Export symbols internally.
> * tests/commandtest.c: New test.
> * tests/Makefile.am (check_PROGRAMS): Run it.
> * tests/commandhelper.c: Auxiliary program.
> * tests/commanddata/test2.log - test15.log: New expected outputs.
> * cfg.mk (useless_free_options): Add virCommandFree.
> * po/POTFILES.in: New translation.
> * .x-sc_avoid_write: Add exemption.
> * tests/.gitignore: Ignore new built file.
> ---
>
> v2: add virCommandTransferFD, virCommandToString, virCommandAddArgFormat,
> virCommandNewArgList, virCommandWriteArgLog, virCommandNonblockingFDs.
> Fix virCommandRunAsync and virCommandFree to free transfered FDs.
> Add a bit more test exposure.
>
> diff --git a/cfg.mk b/cfg.mk
> index dea8301..6312632 100644
> --- a/cfg.mk
> +++ b/cfg.mk
> @@ -77,6 +77,7 @@ useless_free_options = \
> --name=virCapabilitiesFreeHostNUMACell \
> --name=virCapabilitiesFreeMachines \
> --name=virCgroupFree \
> + --name=virCommandFree \
> --name=virConfFreeList \
> --name=virConfFreeValue \
> --name=virDomainChrDefFree \
You should also add virCommandError to the msg_gen_function list.
> diff --git a/src/util/command.c b/src/util/command.c
> new file mode 100644
> index 000..948a957
> --- /dev/null
> +++ b/src/util/command.c
> +/*
> + * Create a new command with a NULL terminated
> + * set of args, taking binary from argv[0]
> + */
s/argv[0]/args[0]/
> +virCommandPtr
> +virCommandNewArgs(const char *const*args)
> +
> +/*
> + * Capture the child's stdout to a string buffer
> + */
> +void
> +virCommandSetOutputBuffer(virCommandPtr cmd, char **outbuf)
> +{
> + if (!cmd || cmd->has_error)
> + return;
> +
> + if (cmd->outfd != -1) {
To detect double assignment properly you need to check for this I think:
if (cmd->outfd != -1 || cmd->outfdptr || cmd->outbuf) {
> + cmd->has_error = -1;
> + VIR_DEBUG0("cannot specify output twice");
> + return;
> + }
> +
> + cmd->outbuf = outbuf;
> + cmd->outfdptr = &cmd->outfd;
> +}
> +
> +
> +/*
> + * Capture the child's stderr to a string buffer
> + */
> +void
> +virCommandSetErrorBuffer(virCommandPtr cmd, char **errbuf)
> +{
> + if (!cmd || cmd->has_error)
> + return;
> +
> + if (cmd->errfd != -1) {
The same pattern as in virCommandSetOutputBuffer:
if (cmd->errfd != -1 || cmd->errfdptr || cmd->errbuf) {
> + cmd->has_error = -1;
> + VIR_DEBUG0("cannot specify stderr twice");
> + return;
> + }
> +
> + cmd->errbuf = errbuf;
> + cmd->errfdptr = &cmd->errfd;
> +}
> +
> +
> +/*
> + * Attach a file descriptor to the child's stdin
> + */
> +void
> +virCommandSetInputFD(virCommandPtr cmd, int infd)
> +{
> + if (!cmd || cmd->has_error)
> + return;
> +
> + if (infd < 0 || cmd->inbuf) {
I think you meant to check here for this instead:
if (cmd->infd != -1 || cmd->inbuf) {
> + cmd->has_error = -1;
> + VIR_DEBUG0("cannot specify input twice");
> + return;
> + }
> +
> + cmd->infd = infd;
> +}
> +
> +
> +/*
> + * Attach a file descriptor to the child's stdout
> + */
> +void
> +virCommandSetOutputFD(virCommandPtr cmd, int *outfd)
> +{
> + if (!cmd || cmd->has_error)
> + return;
> +
> + if (!outfd || cmd->outfd != -1) {
I think you meant to check here for this instead:
if (cmd->outfd != -1 || cmd->outfdptr || cmd->outbuf) {
> + cmd->has_error = -1;
> + VIR_DEBUG0("cannot specify output twice");
> + return;
> + }
> +
> + cmd->outfdptr = outfd;
> +}
> +
> +
> +/*
> + * Attach a file descriptor to the child's stderr
> + */
> +void
> +virCommandSetErrorFD(virCommandPtr cmd, int *errfd)
> +{
> + if (!cmd || cmd->has_error)
> + return;
> +
> + if (!errfd || cmd->errfd != -1) {
I think you meant to check here for this instead:
if (cmd->errfd != -1 || cmd->errfdptr || cmd->errbuf) {
> + cmd->has_error = -1;
> + VIR_DEBUG0("cannot specify stderr twice");
> + return;
> + }
> +
> + cmd->errfdptr = errfd;
> +}
> +
> +/*
> + * Call after adding all arguments and environment settings, but before
> + * Run/RunAsync, to immediately output the environment and arguments of
> + * cmd to logfd. If virCommandRun cannot succeed (because of an
> + * out-of-memory condition while building cmd), nothing will be logged.
> + */
> +void
> +virCommandWriteArgLog(virCommandPtr cmd, int logfd)
> +{
> + in
