[libvirt] [PATCH java] Add support for Libvirt Qemu Library
This allows us to send Qemu Guest Agent commands to running domains. Signed-off-by: Wido den Hollander <wido@widodh.nl> --- 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 @@ -8,6 +8,7 @@ import org.libvirt.jna.CString; import org.libvirt.jna.DomainPointer; import org.libvirt.jna.DomainSnapshotPointer; import org.libvirt.jna.Libvirt; +import org.libvirt.jna.LibvirtQemu; import org.libvirt.jna.SizeT; import org.libvirt.jna.virDomainBlockInfo; import org.libvirt.jna.virDomainBlockStats; @@ -22,6 +23,7 @@ import org.libvirt.event.LifecycleListener; import org.libvirt.event.PMWakeupListener; import org.libvirt.event.PMSuspendListener; import static org.libvirt.Library.libvirt; +import static org.libvirt.Library.libvirtqemu; import static org.libvirt.ErrorHandler.processError; import static org.libvirt.ErrorHandler.processErrorIfZero; @@ -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; + } + /** * the native virDomainPtr. */ @@ -1556,4 +1575,21 @@ public class Domain { return processError(libvirt.virDomainUpdateDeviceFlags(VDP, xml, flags)); } + /** + * Send a Qemu Guest Agent command to a domain + * + * @param cmd + * The command which has to be send + * @param timeout + * The timeout for waiting on an answer. See QemuAgentFlags for more information. + * @param flags + * Flags to be send + * @return String + * @throws LibvirtException + */ + public String qemuAgentCommand(String cmd, int timeout, int flags) throws LibvirtException { + CString result = libvirtqemu.virDomainQemuAgentCommand(this.VDP, cmd, timeout, flags); + processError(result); + return result.toString(); + } } 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; } private Library() {} diff --git a/src/main/java/org/libvirt/jna/LibvirtQemu.java b/src/main/java/org/libvirt/jna/LibvirtQemu.java new file mode 100644 index 0000000..82213e9 --- /dev/null +++ b/src/main/java/org/libvirt/jna/LibvirtQemu.java @@ -0,0 +1,16 @@ +package org.libvirt.jna; + +import com.sun.jna.Library; +import com.sun.jna.Native; +import com.sun.jna.Platform; + +/** + * The libvirt Qemu interface which is exposed via JNA + */ + +public interface LibvirtQemu extends Library { + + LibvirtQemu INSTANCE = (LibvirtQemu) Native.loadLibrary(Platform.isWindows() ? "virt-qemu-0" : "virt-qemu", LibvirtQemu.class); + + CString virDomainQemuAgentCommand(DomainPointer virDomainPtr, String cmd, int timeout, int flags); +} -- 1.9.1
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@widodh.nl> --- 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?
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? -- Claudio --
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@widodh.nl> --- 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.
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. Wido
-- Claudio
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@widodh.nl> --- 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. 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. 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?
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). -- Claudio--
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@widodh.nl> --- 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
At Mon, 16 Nov 2015 21:26:57 +0100, Wido den Hollander wrote:
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.
Yes, but that doesn't mean we should not improve it, IMO. Thankfully, in Java, we're not bound by the limitations of the C language. Ease of use of the API is one of my foremost goals for libvirt-java. A plain mapping of the C API functions would be far easier, but also downright ugly (with the Java goggles on). Having a (bit) of Haskell background, and being a Scala programmer by day, a plain String in place of a proper type is just ugly as well, as it does not carry any information as to what it's type really is and what it's format is. I see that the JSON structure of an agent command is pretty simple, but we would have to support strings and numbers. I'd still very much prefer a QemuCommand helper class. If it turns out that it is not flexible enough, we can always add a loophole later, e.g.: class QemuCommand { public static QemuCommand execute(String cmd, /* TBD */ args...) { } public static QemuCommand raw(String jsonObject) { } }
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.
OK, I'm sure we can work that out. Basically, we probably need to access the qemu library through a static method, so we avoid loading the library when it is not necessary. Let me think about that a bit more. -- Claudio--
On 11/17/2015 08:43 AM, Claudio Bley wrote:
At Mon, 16 Nov 2015 21:26:57 +0100, Wido den Hollander wrote:
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.
Yes, but that doesn't mean we should not improve it, IMO. Thankfully, in Java, we're not bound by the limitations of the C language. Ease of use of the API is one of my foremost goals for libvirt-java. A plain mapping of the C API functions would be far easier, but also downright ugly (with the Java goggles on).
Having a (bit) of Haskell background, and being a Scala programmer by day, a plain String in place of a proper type is just ugly as well, as it does not carry any information as to what it's type really is and what it's format is.
I see that the JSON structure of an agent command is pretty simple, but we would have to support strings and numbers. I'd still very much prefer a QemuCommand helper class.
True, I understand. Looking at this I would want to use the JSONReader/JsonWriter from Java, but that's not available by default. We probably don't want any external libs for libvirt-java. 'arguments' for Qemu GA is rather simple, it's an array with Strings or Ints. I rather not write my own JsonWriter, so any ideas here?
If it turns out that it is not flexible enough, we can always add a loophole later, e.g.:
Indeed. We could also implement all available commands as a Enum: http://git.qemu.org/?p=qemu.git;a=blob;f=qga/qapi-schema.json;h=78362e071d18... It's just the arguments I'm stuck with. I have a few commits, do you care to take a look at them: https://github.com/wido/libvirt-java/commits/qemu-guest-command Especially this one: https://github.com/wido/libvirt-java/commit/1ff76ed0a8f8d58a102cd4e2bc948df9... Wido
class QemuCommand { public static QemuCommand execute(String cmd, /* TBD */ args...) { }
public static QemuCommand raw(String jsonObject) { } }
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.
OK, I'm sure we can work that out. Basically, we probably need to access the qemu library through a static method, so we avoid loading the library when it is not necessary. Let me think about that a bit more.
-- Claudio--
participants (2)
-
Claudio Bley -
Wido den Hollander