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@xxxxxxxxxx +1-919-301-3266 Libvirt virtualization library http://libvirt.org
Attachment:
signature.asc
Description: OpenPGP digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list