[libvirt] [PATCH 2/3] Support loading a configuration file for sanlock plugin

[Daniel Veillard]

On Fri, Jun 17, 2011 at 01:38:20PM +0100, Daniel P. Berrange wrote:
> Introduce a configuration file with a single parameter
> 'require_lease_for_disks', which is used to decide whether
> it is allowed to start a guest which has read/write disks,
> but without any leases.
[...]
> @@ -62,22 +72,76 @@ struct _virLockManagerSanlockPrivate {
>  /*
>   * sanlock plugin for the libvirt virLockManager API
>   */
> +static int virLockManagerSanlockLoadConfig(const char *configFile)
[...]
> +    if (!(conf = virConfReadFile(configFile, 0)))
> +        return -1;
> +
> +#define CHECK_TYPE(name,typ) if (p && p->type != (typ)) {               \
> +        virLockError(VIR_ERR_INTERNAL_ERROR,                            \
> +                     "%s: %s: expected type " #typ,                     \
> +                     configFile, (name));                               \
> +        virConfFree(conf);                                              \
> +        return -1;                                                      \
> +    }
> +
> +    p = virConfGetValue(conf, "require_lease_for_disks");
> +    CHECK_TYPE("require_lease_for_disks", VIR_CONF_LONG);
> +    if (p)
> +        driver->requireLeaseForDisks = p->l;
> +
> +    virConfFree(conf);
> +    return 0;
> +}

  Hum, defining a complex macro in a function body is fine if you tend
  to reuse that macro a lot, but for a single use it makes the code
  harder to read. So unless you really expect extensions or reuse (in
  which case it should be defined somewhere else) I'm not sure it's
  a good idea here :-)

[...]
> diff --git a/src/locking/sanlock.conf b/src/locking/sanlock.conf
> new file mode 100644
> index 0000000..818b529
> --- /dev/null
> +++ b/src/locking/sanlock.conf
> @@ -0,0 +1,6 @@
> +#
> +# Flag to determine whether we allow starting of guests
> +# which do not have any <lease> elements defined in their
> +# configuration.
> +#
> +#require_lease_for_disks = 1
> diff --git a/src/locking/test_libvirt_sanlock.aug b/src/locking/test_libvirt_sanlock.aug
> new file mode 100644
> index 0000000..2f1f57a
> --- /dev/null
> +++ b/src/locking/test_libvirt_sanlock.aug
> @@ -0,0 +1,7 @@
> +module Test_libvirt_sanlock =
> +
> +   let conf = "require_lease_for_disks = 1
> +"
> +
> +   test Libvirt_sanlock.lns get conf =
> +{ "require_lease_for_disks" = "1" }

  Okay, ACK, but look again at this macro, do your really want this in ?

Daniel

-- 
Daniel Veillard      | libxml Gnome XML XSLT toolkit  http://xmlsoft.org/
daniel at veillard.com  | Rpmfind RPM search engine http://rpmfind.net/
http://veillard.com/ | virtualization library  http://libvirt.org/