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

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?

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