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--