On 07/13/2012 08:02 AM, Michal Privoznik wrote: > On 13.07.2012 15:41, Eric Blake wrote: >> On 07/13/2012 06:25 AM, Michal Privoznik wrote: >>> On 09.07.2012 12:54, Michal Privoznik wrote: >>>> [I am pasting your patch here again so I can point out some nits] >>>> >>>> On 06.07.2012 03:29, MATSUDA, Daiki wrote: >>>>> >>> >>>> >>>> However, I will hold the push for a while. There is one general problem >>>> with this patch. Unlike qemu monitor, guest agent has several commands >>>> which don't return any successful message (guest-suspend-* family). with >>>> current implementation libvirt takes an event on command as confirmation >>>> of success. That is - we block until an event is received. But with this >>>> patch, if user will issue such command the whole API would block >>>> endlessly. I am not sure how we should fix this. Either we will take the >>>> current implementation (what if qemu will ever come with new command which >>>> doesn't report successful run?) or we introduce a flag DONT_WAIT_FOR_REPLY >>>> (tricky, we want to wait for error response - but what timeout should we >>>> choose? moreover, timeouts are bad). >>>> >>>> Therefore, if anybody has any suggestions I am more than happy to hear >>>> them. >>> >>> What about this: by default there will be some reasonable timeout (like >>> 30 seconds) for obtaining a reply from agent. And we will have two flags: >>> 1) DONT_WAIT - which will just write command and don't wait for any reply >>> 2) WAIT_INF - which will block endlessly for a reply. >> >> 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).
>
> Michal
>
>
--
Eric Blake [email protected] +1-919-301-3266
Libvirt virtualization library http://libvirt.org
signature.asc
Description: OpenPGP digital signature
-- libvir-list mailing list [email protected] https://www.redhat.com/mailman/listinfo/libvir-list
