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

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