[libvirt] [PATCH 3/4] Support automatic creation of leases for disks in sanlock

[Daniel P. Berrange]

On Tue, Jun 28, 2011 at 11:02:24AM -0600, Eric Blake wrote:
> > +    if (virAsprintf(&path, "%s/%s",
> > +                    driver->autoDiskLeasePath,
> > +                    VIR_LOCK_MANAGER_SANLOCK_AUTO_DISK_LOCKSPACE) < 0) {
> > +        virReportOOMError();
> > +        goto error;
> > +    }
> > +
> > +    memcpy(ls.name, VIR_LOCK_MANAGER_SANLOCK_AUTO_DISK_LOCKSPACE, SANLK_NAME_LEN);
> > +    ls.host_id = 0; /* Doesn't matter for initialization */
> > +    ls.flags = 0;
> > +    memcpy(&ls.host_id_disk, path, sizeof(struct sanlk_disk));
> 
> Dangerous.  This could copy more bytes than strlen(path) and cross a
> page boundary, or it could silently truncate if strlen(path) is longer
> than sizeof(struct sanlk_disk).  I think you need to use virStrncpy here.

That line is so bogus. It has been replace with


    if (!virStrcpy(ls.host_id_disk.path, path, SANLK_NAME_LEN)) {
        virLockError(VIR_ERR_INTERNAL_ERROR,
                     _("Lockspace path '%s' exceeded %d characters"),
                     path, SANLK_NAME_LEN);
        goto error;
    }


> > +# When automatically creating leases for disks, each host which
> > +# can access the filesystem mounted at 'disk_lease_dir' will need
> > +# to have a unique host ID assigned. The 'max_hosts' parameter
> > +# specifies an upper limit on the number of participating hosts.
> > +#
> > +# Each additional host requires 1 sector of disk space, usually
> > +# 512 bytes. The default is 64, and can be reduced if you don't
> > +# have many hosts, or increased if you have more.
> > +#
> > +#max_hosts = 64
> 
> Weren't there some list comments about either dropping this parameter,
> or changing it to 2000, and that sanlock has changed to use more than
> 512 bytes per host now?
> http://www.redhat.com/archives/libvir-list/2011-June/msg01237.html

Yes, I've killed off 'max_hosts' now.

> 
> > +++ b/tools/Makefile.am
> > @@ -12,6 +12,7 @@ EXTRA_DIST = \
> >  	$(ICON_FILES)					\
> >  	virt-xml-validate.in				\
> >  	virt-pki-validate.in				\
> > +	virt-sanlock-cleanup.in				\
> >  	virsh.pod					\
> >  	libvirt-guests.init.sh				\
> >  	libvirt-guests.sysconf
> > @@ -19,8 +20,14 @@ EXTRA_DIST = \
> >  bin_SCRIPTS = virt-xml-validate virt-pki-validate
> >  bin_PROGRAMS = virsh
> >  
> > -dist_man1_MANS = virt-xml-validate.1 virt-pki-validate.1 virsh.1
> > +if HAVE_SANLOCK
> > +sbin_SCRIPTS = virt-sanlock-cleanup
> > +endif
> >  
> > +dist_man1_MANS = virt-xml-validate.1 virt-pki-validate.1 virsh.1
> > +if HAVE_SANLOCK
> > +dist_man8_MANS = virt-sanlock-cleanup.8
> 
> Ouch.  Conditional distribution...
> 
> > +endif
> >  
> >  virt-xml-validate: virt-xml-validate.in Makefile
> >  	$(AM_V_GEN)sed -e 's,[@]SCHEMADIR@,$(pkgdatadir)/schemas,' < $< > $@ \
> > @@ -36,6 +43,14 @@ virt-pki-validate: virt-pki-validate.in Makefile
> >  virt-pki-validate.1: virt-pki-validate.in
> >  	$(AM_V_GEN)$(POD2MAN) $< $(srcdir)/$@
> >  
> > +virt-sanlock-cleanup: virt-sanlock-cleanup.in Makefile
> > +	$(AM_V_GEN)sed -e 's,[@]SYSCONFDIR@,$(sysconfdir),' \
> > +	    -e 's,[@]LOCALSTATEDIR@,$(localstatedir),' < $< > $@ \
> > +	    || (rm $@ && exit 1) && chmod +x $@
> > +
> > +virt-sanlock-cleanup.8: virt-sanlock-cleanup.in
> > +	$(AM_V_GEN)$(POD2MAN) $< $(srcdir)/$@
> 
> ...that depends on perl.  Commit 6db98a2d4b previously fixed a bug just
> like this; the idea being that we have to unconditionally build
> virt-sanlock-cleanup.8.in using $(POD2MAN) and put that in the tarball,
> then conditionally build virt-sanlock-cleanup.8 using only sed, so that
> end users can run 'make dist' regardless of configure options and can
> still avoid perl.
> 
> But since I wrote commit 6db98a2d4b, I'm happy to clean this up as a
> separate followup patch, if you'd like.

Ok, I'll let you tackle it

> > +# A script to cleanup resource leases auto-created by
> > +# the libvirt lock plugin for sanlock
> > +
> > +verbose=1
> > +if test "$1" = "-q" ; then
> 
> if test "x$1" = "x-q" ; then
> 
> The leading x is necessary, to prevent test from going haywire if $1
> happens to be something like [.

Done that

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