[libvirt] [PATCH] qemu: make sure agent returns error when required data are missing

[Martin Kletzander]

On Thu, Apr 03, 2014 at 09:32:02AM +0200, Peter Krempa wrote:
>On 04/03/14 07:58, Martin Kletzander wrote:
>> Commit 5b3492fa aimed to fix this and caught one error but exposed
>> another one.  When agent command is being executed and the thread
>> waiting for the reply is woken up by an event (e.g. EOF in case of
>> shutdown), the command finishes with no data (rxObject == NULL), but
>> no error is reported, since this might be desired by the caller
>> (e.g. suspend through agent).  However, in other situations, when the
>> data are required (e.g. getting vCPUs), we proceed to getting desired
>> data out of the reply, but none of the virJSON*() functions works well
>> with NULLs.  I chose the way of a new parameter for qemuAgentCommand()
>> function that specifies whether reply is required and behaves
>> according to that.
>>
>> Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1058149
>>
>> Signed-off-by: Martin Kletzander <mkletzan at redhat.com>
>> ---
>>
>> Notes:
>>     This issue probably exists since qemu-ga is supported in libvirt, so
>>     this (along with 5b3492fa and e9d09fe1) might be worth back-porting to
>>     some maintenance branches, I just haven't gone through them to see
>>     which ones are applicable.
>>
>>  src/qemu/qemu_agent.c | 19 ++++++++++---------
>>  1 file changed, 10 insertions(+), 9 deletions(-)
>>
>> diff --git a/src/qemu/qemu_agent.c b/src/qemu/qemu_agent.c
>> index 92573bd..4082331 100644
>> --- a/src/qemu/qemu_agent.c
>> +++ b/src/qemu/qemu_agent.c
>> @@ -1080,6 +1080,7 @@ static int
>>  qemuAgentCommand(qemuAgentPtr mon,
>>                   virJSONValuePtr cmd,
>>                   virJSONValuePtr *reply,
>> +                 bool needReply,
>>                   int seconds)
>>  {
>
>Seeing that the "needReply" parameter is true in most cases, I'd rename
>this function and add one not requiring the new parameter for normal
>usage and use the renamed in the few cases that actually expect the VM
>to quit.
>

I don't think it's worth adding this much of unnecessary code just to
eliminate one argument in 4 out of 7 call, especially when all the
functions are private and even static, therefore not much of a cleanup
from the readability POV.

>But this is just a cosmetic issue.
>

Patches are welcome, though ;)

>ACK
>

Thanks, pushed

>Peter
>

Martin
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20140403/3fb38f60/attachment-0001.sig>