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

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

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

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

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