Re: [libvirt] [PATCH] qemu_agent: support qemu agent general command

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 > > -- libvir-list mailing list libvir-list(a)redhat.com https://www.redhat.com/mailman/listinfo/libvir-list