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

[claudio.bley@xxxxxxxxx (Claudio Bley)]

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?

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

--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list