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

[Eric Blake]

On 06/17/2011 06:38 AM, 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.
> 
> * libvirt.spec.in: Add sanlock config file and augeas
>   lens
> * src/Makefile.am: Install sanlock config file and
>   augeas lens
> * src/locking/libvirt_sanlock.aug: Augeas master lens
> * src/locking/test_libvirt_sanlock.aug: Augeas test file
> * src/locking/sanlock.conf: Example sanlock config
> * src/locking/lock_driver_sanlock.c: Wire up loading
>   of configuration file

Where do we mention this file in the documentation?

>  # Build our version script.  This is composed of three parts:
> @@ -1184,9 +1188,26 @@ lockdriver_LTLIBRARIES = sanlock.la
>  sanlock_la_SOURCES = $(LOCK_DRIVER_SANLOCK_SOURCES)
>  sanlock_la_CFLAGS = $(AM_CLFAGS)
>  sanlock_la_LDFLAGS = -module -avoid-version
> -sanlock_la_LIBADD = -lsanlock
> +sanlock_la_LIBADD = -lsanlock \
> +		../gnulib/lib/libgnu.la
> +
> +augeas_DATA += locking/libvirt_sanlock.aug
> +augeastest_DATA += locking/test_libvirt_sanlock.aug
> +
> +EXTRA_DIST += locking/sanlock.conf \
> +    locking/libvirt_sanlock.aug \
> +    locking/test_libvirt_sanlock.aug
> +
> +$(builddir)/locking/%-sanlock.conf: $(srcdir)/locking/sanlock.conf
> +	$(AM_V_GEN)mkdir locking ; \
> +	cp $< $@

What's the purpose of this rule?  We already require GNU make, which is
smart enough to look in $(srcdir) if a file is not present in
$(builddir) during a VPATH build.  In other words, this is looking a bit
more complex than necessary.

> @@ -62,22 +72,76 @@ struct _virLockManagerSanlockPrivate {
>  /*
>   * sanlock plugin for the libvirt virLockManager API
>   */
> +static int virLockManagerSanlockLoadConfig(const char *configFile)
> +{
> +    virConfPtr conf;
> +    virConfValuePtr p;
> +
> +    if (access(configFile, R_OK) == -1) {
> +        if (errno != ENOENT) {
> +            virReportSystemError(errno,
> +                                 _("Unable to access config file %s"),
> +                                 configFile);
> +            return -1;
> +        }
> +        return 0;

So a missing conf file is silently treated as success?

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

I'm not sure we've been doing the best job at being consistent, but when
showing a comment describing an option, is it better to show the default
(where uncommenting makes no changes) or the converse (where
uncommenting instantly provides the most common non-default)?  Thus,
it's probably worth documenting whether the default is 0 or 1.

-- 
Eric Blake   eblake at redhat.com    +1-801-349-2682
Libvirt virtualization library http://libvirt.org

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 619 bytes
Desc: OpenPGP digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20110623/4672e0ec/attachment-0001.sig>