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

[Claudio Bley]

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