On 08/05/2012 04:49 PM, MATSUDA, Daiki wrote:
ACK or do I need to re-write ?
Given this earlier review...
Rather than just 2 flags, can we take an integer timeout parameter? If the parameter is -1, then we don't wait; if the parameter is 0, then we wait forever, otherwise, we wait for the user-specified timeout.
Okay, but I think the interpretation of values should be reversed: -1 for wait forever 0 not to wait at all
I think it's more mnemonic since -1 is all bits set thus huge number hence evokes infinity. Wait for zero time units means no wait at all.
Definitely argues that -1 and 0 should have named aliases.
Or even use the NULL-ness of result to determine whether to wait:
/** * virDomainQemuAgentCommand: * * Issue @cmd to the guest agent running in @domain. * If @result is NULL, then don't wait for a result (and @timeout * must be 0). Otherwise, wait for @timeout seconds for a * successful result to be populated into *@result, with * VIR_DOMAIN_QEMU_AGENT_COMMAND_BLOCK (-1) meaning to block * forever waiting for a result. */ int virDomainQemuAgentCommand(virDomainPtr domain, const char *cmd, char **result, unsigned int timeout, unsigned int flags);
And I still stand by my earlier review comment that this needs to be split into multiple patches (introduce public API separate from the qemu-specific implementation of that API, even if the API is only usable by qemu).
I think that the state of this patch right now is waiting for you to do a rewrite that splits it into several patches and takes into account the review comments on altering the API. It's still a good idea, but I'm personally a bit swamped to take over the series myself without first seeing a more up-to-date posting. -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org