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

[Daniel P. Berrange]

On Thu, Jun 23, 2011 at 08:05:46AM -0600, Eric Blake wrote:
> 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?

I've got a new patch which adds docs to the website.

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

I'm not sure what you mean ? This rules generates qemu-sanlock.conf
from sanlock.conf, so how can we do without it ?

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

Yep, that's the way we deal with libvirtd.conf and qemu.conf
too.

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

Yep, that's in the next patch

Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|