Re: [PATCH java] Add support for Libvirt Qemu Library

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




On 11/15/2015 11:26 PM, Claudio Bley wrote:
> At Fri, 13 Nov 2015 14:59:52 +0100,
> Wido den Hollander wrote:
>>
>>
>>
>> On 12-11-15 23:04, Claudio Bley wrote:
>>> Hi.
>>>
>>> At Mon,  9 Nov 2015 13:48:18 +0100,
>>> Wido den Hollander wrote:
>>>>
>>>> This allows us to send Qemu Guest Agent commands to running domains.
>>>>
>>>> Signed-off-by: Wido den Hollander <wido@xxxxxxxxx>
>>>> ---
>>>>  src/main/java/org/libvirt/Domain.java          | 36 ++++++++++++++++++++++++++
>>>>  src/main/java/org/libvirt/Library.java         |  3 +++
>>>>  src/main/java/org/libvirt/jna/LibvirtQemu.java | 16 ++++++++++++
>>>>  3 files changed, 55 insertions(+)
>>>>  create mode 100644 src/main/java/org/libvirt/jna/LibvirtQemu.java
>>>>
>>>> diff --git a/src/main/java/org/libvirt/Domain.java b/src/main/java/org/libvirt/Domain.java
>>>> index 83a500c..c24df48 100644
>>>> --- a/src/main/java/org/libvirt/Domain.java
>>>> +++ b/src/main/java/org/libvirt/Domain.java
>>>> @@ -141,6 +143,23 @@ public class Domain {
>>>>          public static final int NO_METADATA = (1 << 4);
>>>>      }
>>>>  
>>>> +    static final class QemuAgentFlags {
>>>> +        /**
>>>> +         * Do not wait for a result
>>>> +         */
>>>> +        public static final int VIR_DOMAIN_QEMU_AGENT_COMMAND_NOWAIT = 0;
>>>> +
>>>> +        /**
>>>> +         * Use default timeout value
>>>> +         */
>>>> +        public static final int VIR_DOMAIN_QEMU_AGENT_COMMAND_DEFAULT = -1;
>>>> +
>>>> +        /**
>>>> +         * Block forever waiting for a result
>>>> +         */
>>>> +        public static final int VIR_DOMAIN_QEMU_AGENT_COMMAND_BLOCK = -2;
>>>> +    }
>>>> +
>>>
>>> I would much prefer an Enum instead of some integer constants. Also,
>>> those flags are currently of little use since the QemuAgentFlags class
>>> is package private.
>>>
>>> Also, which version are you targeting? Seems there's also a shutdown flag:
>>>
>>> typedef enum {
>>>     VIR_DOMAIN_QEMU_AGENT_COMMAND_MIN = -2,
>>>     VIR_DOMAIN_QEMU_AGENT_COMMAND_BLOCK = -2,
>>>     VIR_DOMAIN_QEMU_AGENT_COMMAND_DEFAULT = -1,
>>>     VIR_DOMAIN_QEMU_AGENT_COMMAND_NOWAIT = 0,
>>>     VIR_DOMAIN_QEMU_AGENT_COMMAND_SHUTDOWN = 60,
>>> } virDomainQemuAgentCommandTimeoutValues;
>>>
>>> And why are those flags mixed up? Seems like a bad idea to me. Are
>>> those "values" intended to go into the "timeout" parameter or into the
>>> flags?
>>>
>>> And since which release is this actually available in libvirt?
>>>
>>
>> Hmm, I looked at the master branch on Git. I didn't find the ones you
>> send. Let me work on that.
> 
> They are defined in include/libvirt/libvirt-qemu.h.
> 
> Apparently, those constants are special timeout values and the
> function does not take any flags currently. So the name
> "QemuAgentFlags" was a bit misleading.
> 
> The SHUTDOWN value is merely an internal value in case when you want
> to shut down the guest via the agent, basically an educated guess that
> a 60 seconds timeout value should work for a shut down in most cases.
> 
> See dd725c53e905db51f39dbaa4a0673e8d1588301b
> 
> Since there are no flags currently, simply drop the flags
> parameter. We add flags (as enums) later when the function starts to
> support one.
> 

Good point, I'll come up with a revised patch.

> Furthermore, I think I'm in favor of adding a handful of methods
> instead of (ab)using some special values of a parameter.
> 
> /* blocking call */
> String qemuAgentCommand(String cmd)
> 
> /* default timeout call */
> String qemuAgentCommandDefaultTimeout(String cmd)
> 
> /* timeout in seconds. no special 0 constant needed. */
> String qemuAgentCommandTimeout(String cmd, int timeoutSeconds) {
>   if (timeoutSeconds < 0) throw IllegalArgumentException("timeoutSeconds value must be >= 0");
> 
>   ...
> }
> 
> Do you have a link to some docs where the available commands are
> described? We should add this to the javadoc.
> 

Yes, here they are: http://wiki.qemu.org/Features/QAPI/GuestAgent

> What does a qemu command look like? It seems it's some kind of JSON
> object. In that case, I think we should not make every user of the
> library start to construct their own JSON strings.
> 
> May be a simple utility class, e.g.
> 
> public class QemuCommand {
>   public static QemuCommand create(String cmd, String args...) {
>     ...
>   }
> }
> 
> should be added and the qemuAgentCommand changed to take such a
> parameter instead?
> 

Hmm, I don't know if we want to do that. I think we should always keep
the option open for the user. Libvirt doesn't provide it. We are only
mapping libvirt functions.

I also have some example code:
https://gist.github.com/wido/30457cbe66e3965f00de

I used that for testing.

>>>> diff --git a/src/main/java/org/libvirt/Library.java
> b/src/main/java/org/libvirt/Library.java
>>>> index 8e054c6..30f15be 100644
>>>> --- a/src/main/java/org/libvirt/Library.java
>>>> +++ b/src/main/java/org/libvirt/Library.java
>>>> @@ -2,6 +2,7 @@ package org.libvirt;
>>>>  
>>>>  import org.libvirt.jna.Libvirt;
>>>>  import org.libvirt.jna.Libvirt.VirEventTimeoutCallback;
>>>> +import org.libvirt.jna.LibvirtQemu;
>>>>  import org.libvirt.jna.CString;
>>>>  import static org.libvirt.ErrorHandler.processError;
>>>>  
>>>> @@ -34,6 +35,7 @@ public final class Library {
>>>>          };
>>>>  
>>>>      final static Libvirt libvirt;
>>>> +    final static LibvirtQemu libvirtqemu;
>>>>  
>>>>      // an empty string array constant
>>>>      // prefer this over creating empty arrays dynamically.
>>>> @@ -47,6 +49,7 @@ public final class Library {
>>>>          } catch (Exception e) {
>>>>              e.printStackTrace();
>>>>          }
>>>> +        libvirtqemu = LibvirtQemu.INSTANCE;
>>>>      }
>>>
>>> I doubt that we always should load that library. In case the user has
>>> an old version this would fail, wouldn't it?
>>>
>>
>> Yes, that would indeed fail. What do you suggest? Load when required? My
>> JNA foo is not that good to get this implemented.
> 
> We can probably use some sort of singleton pattern
> (e.g. initialization-on-demand holder).
> 

I might need some help with that! I'll come up with a revised patched
based on your comments.

Wido

--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list



[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]