On 25.01.2013 19:26, Peter Krempa wrote:
> On 01/23/13 10:41, Michal Privoznik wrote:
>> Currently, if we want to feed stdin, or catch stdout or stderr of a
>> virCommand we have to use virCommandRun(). When using virCommandRunAsync()
>> we have to register FD handles by hand. This may lead to code duplication.
>> Hence, introduce an internal API, which does this automatically within
>> virCommandRunAsync(). The intended usage looks like this:
>>
>> virCommandPtr cmd = virCommandNew*(...);
>> char *buf = NULL;
>>
>> ...
>>
>> virCommandSetOutputBuffer(cmd, &buf);
>> virCommandDoAsyncIO(cmd);
>>
>> if (virCommandRunAsync(cmd, NULL) < 0)
>> goto cleanup;
>>
>> ...
>>
>> if (virCommandWait(cmd, NULL) < 0)
>> goto cleanup;
>>
>> /* @buf now contains @cmd's stdout */
>> VIR_DEBUG("STDOUT: %s", NULLSTR(buf));
>>
>> ...
>>
>> cleanup:
>> VIR_FREE(buf);
>> virCommandFree(cmd);
>>
>> Note, that both stdout and stderr buffers may change until
>> virCommandWait()
>> returns.
>> ---
>> src/libvirt_private.syms | 1 +
>> src/util/vircommand.c | 214 +++++++++++++++++++++++++++++++++++++++++++++--
>> src/util/vircommand.h | 1 +
>> 3 files changed, 208 insertions(+), 8 deletions(-)
>>
>> diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
>> index fc23adc..f89d1aa 100644
>> --- a/src/libvirt_private.syms
>> +++ b/src/libvirt_private.syms
>> @@ -142,6 +142,7 @@ virCommandAddEnvString;
>> virCommandAllowCap;
>> virCommandClearCaps;
>> virCommandDaemonize;
>> +virCommandDoAsyncIO;
>> virCommandExec;
>> virCommandFree;
>> virCommandHandshakeNotify;
>> diff --git a/src/util/vircommand.c b/src/util/vircommand.c
>> index 8566d1a..5d67bd2 100644
>> --- a/src/util/vircommand.c
>> +++ b/src/util/vircommand.c
>> @@ -47,11 +47,12 @@
>>
>> /* Flags for virExecWithHook */
>> enum {
>> - VIR_EXEC_NONE = 0,
>> - VIR_EXEC_NONBLOCK = (1 << 0),
>> - VIR_EXEC_DAEMON = (1 << 1),
>> + VIR_EXEC_NONE = 0,
>> + VIR_EXEC_NONBLOCK = (1 << 0),
>> + VIR_EXEC_DAEMON = (1 << 1),
>> VIR_EXEC_CLEAR_CAPS = (1 << 2),
>> - VIR_EXEC_RUN_SYNC = (1 << 3),
>> + VIR_EXEC_RUN_SYNC = (1 << 3),
>> + VIR_EXEC_ASYNC_IO = (1 << 4),
>> };
>>
>> struct _virCommand {
>> @@ -84,6 +85,11 @@ struct _virCommand {
>> int *outfdptr;
>> int *errfdptr;
>>
>> + size_t inbufOffset;
>> + int inWatch;
>> + int outWatch;
>> + int errWatch;
>> +
>> bool handshake;
>> int handshakeWait[2];
>> int handshakeNotify[2];
>> @@ -779,6 +785,7 @@ virCommandNewArgs(const char *const*args)
>> cmd->handshakeNotify[1] = -1;
>>
>> cmd->infd = cmd->outfd = cmd->errfd = -1;
>> + cmd->inWatch = cmd->outWatch = cmd->errWatch = -1;
>> cmd->pid = -1;
>>
>> virCommandAddArgSet(cmd, args);
>> @@ -2122,6 +2129,152 @@ virCommandHook(void *data)
>> }
>>
>>
>> +static void
>> +virCommandHandleReadWrite(int watch, int fd, int events, void *opaque)
>> +{
>> + virCommandPtr cmd = (virCommandPtr) opaque;
>> + char **bufptr = NULL;
>> + char buf[1024];
>> + ssize_t nread;
>> + size_t len = 0;
>> + int *watchPtr = NULL;
>> +
>> + VIR_DEBUG("watch=%d fd=%d events=%d", watch, fd, events);
>> +
>> + if (watch == cmd->inWatch) {
>> + watchPtr = &cmd->inWatch;
>> +
>> + if (events & VIR_EVENT_HANDLE_WRITABLE) {
>> + len = strlen(cmd->inbuf);
>
> I suppose this isn't intended to work on non-string data. It is worth
> documenting this so that it doesn't surprise someone.
>
Yep, I agree. Especially after my (safe-)read issue :)
On the other hand, same strlen() is used in virCommandProcessIO(),
that is virCommandRun(). So we and virCommandRunAsync() are able
to cope with the very same set of string as synchronous variant.
But then again, mentioning this in function description is better way
of threating developers than letting them to have to find out themselves.
>> +
>> + while ((nread = write(fd, cmd->inbuf + cmd->inbufOffset,
>> + len - cmd->inbufOffset))) {
>
> Hm, what's the difference between len and inbuffOffset here?
'len' holds the length of cmd->inbuf, where 'cmd->inbufOffset' says
how much of 'cmd->inbuf' has been written ...
>
>> + if (nread < 0) {
>> + if (errno != EAGAIN)
>> + virReportSystemError(errno,
>> + _("Unable to write command's "
>> + "input to FD %d"),
>> + fd);
>> + break;
>> + }
>> +
>> + cmd->inbufOffset += nread;
... which is why it is updated here.
>> + if (cmd->inbufOffset == len)
>> + VIR_FORCE_CLOSE(cmd->infd);
>> + }
>> + }
>> + } else {
>> + if (watch == cmd->outWatch) {
>> + watchPtr = &cmd->outWatch;
>> + bufptr = cmd->outbuf;
>> + }
>> +
>> + if (watch == cmd->errWatch) {
>> + watchPtr = &cmd->errWatch;
>> + bufptr = cmd->errbuf;
>> + }
>> +
>> + if (bufptr && events & VIR_EVENT_HANDLE_READABLE) {
>> + if (*bufptr)
>> + len = strlen(*bufptr);
>> +
>> + while ((nread = read(fd, buf, sizeof(buf)))) {
>> + if (nread < 0) {
>> + if (errno != EAGAIN)
>> + virReportSystemError(errno,
>> + _("unable to read command's "
>> + "output from FD %d"),
>> + fd);
>> + break;
>> + }
>> +
>> + if (VIR_REALLOC_N(*bufptr, len + nread + 1) < 0) {
>> + virReportOOMError();
>> + break;
>> + }
>> +
>> + memcpy(*bufptr + len, buf, nread);
>> + (*bufptr)[len + nread] = '\0';
>> + }
>> + }
>> + }
>> +
>> + if (events & VIR_EVENT_HANDLE_HANGUP) {
>> + *watchPtr = -1;
>> + virEventRemoveHandle(watch);
>> + }
>> +}
>> +
>> +
>> @@ -2265,6 +2442,11 @@ virCommandWait(virCommandPtr cmd, int *exitstatus)
>> return -1;
>> }
>>
>> + while (cmd->inWatch != -1 &&
>> + cmd->outWatch != -1 &&
>> + cmd->errWatch != -1)
>> + usleep(100);
>
> Hm, is there a possibility to avoid this active wait loop?
I think I can add a condition variable to wait on.
Let me post a v2 and we will see.
>
>> +
>> /* If virProcessWait reaps pid but then returns failure because
>> * exitstatus was NULL, then a second virCommandWait would risk
>> * calling waitpid on an unrelated process. Besides, that error
>> @@ -2516,8 +2698,24 @@ virCommandFree(virCommandPtr cmd)
>> if (cmd->reap)
>> virCommandAbort(cmd);
>>
>> + if (cmd->inWatch != -1)
>> + virEventRemoveHandle(cmd->inWatch);
>> + if (cmd->outWatch != -1)
>> + virEventRemoveHandle(cmd->outWatch);
>> + if (cmd->errWatch != -1)
>> + virEventRemoveHandle(cmd->errWatch);
>> +
>> VIR_FREE(cmd->transfer);
>> VIR_FREE(cmd->preserve);
>>
>> VIR_FREE(cmd);
>> }
>> +
>> +void
>> +virCommandDoAsyncIO(virCommandPtr cmd)
>> +{
>> + if (!cmd || cmd->has_error)
>> + return;
>> +
>> + cmd->flags |= VIR_EXEC_ASYNC_IO;
>> +}
>> diff --git a/src/util/vircommand.h b/src/util/vircommand.h
>> index 9b7117d..c1a2e24 100644
>> --- a/src/util/vircommand.h
>> +++ b/src/util/vircommand.h
>> @@ -163,4 +163,5 @@ void virCommandAbort(virCommandPtr cmd);
>>
>> void virCommandFree(virCommandPtr cmd);
>>
>> +void virCommandDoAsyncIO(virCommandPtr cmd);
>> #endif /* __VIR_COMMAND_H__ */
>>
>
> Peter
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list