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