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 :|
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list