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

[Eric Blake]

On 06/24/2011 07:02 AM, Daniel P. Berrange wrote:
> The current sanlock plugin requires a central management
> application to manually add <lease> elements to each guest,
> to protect resources that are assigned to it (eg writable
> disks). This makes the sanlock plugin useless for usage
> in more adhoc deployment environments where there is no

s/adhoc/ad hoc/

> 
> To make use of this capability the admin will need todo
> several tasks:

s/todo/to do/

> +static int virLockManagerSanlockSetupLockspace(void)
> +{
> +    int fd = -1;
> +    struct stat st;
> +    int rv;
> +    struct sanlk_lockspace ls;
> +    char *path = NULL;
> +
> +    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.

>  
> -static int virLockManagerSanlockAddResource(virLockManagerPtr lock,
> -                                            unsigned int type,
> -                                            const char *name,
> -                                            size_t nparams,
> -                                            virLockManagerParamPtr params,
> -                                            unsigned int flags)
> +
> +static const char hex[] = { '0', '1', '2', '3', '4', '5', '6', '7',
> +                            '8', '9', 'a', 'b', 'c', 'd', 'e', 'f' };

I would have written this:

static const char hex[] = "0123456789abcdef";

but either way works.

> +
> +static int virLockManagerSanlockCreateLease(struct sanlk_resource *res)
> +{
> +    int fd = -1;
> +    struct stat st;
> +    int rv;
> +
> +    if (stat(res->disks[0].path, &st) < 0) {
> +        VIR_DEBUG("Lockspace %s does not yet exist", res->disks[0].path);
> +        if ((fd = open(res->disks[0].path, O_WRONLY|O_CREAT|O_EXCL, 0600)) < 0) {

This feels like common copy-and-paste code; should attaching to the
lockspace be factored into a common helper routine?

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

> +++ b/tools/virt-sanlock-cleanup.in
> @@ -0,0 +1,96 @@
> +#!/bin/sh
> +
> +# 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 [.

> +=head1 EXIT STATUS
> +
> +Upon successful processing of leases cleanup, the exit status
> +will be 0 will be set. Upon fatal eror a non-zero status will

s/eror/error/

ACK with the spelling and memcpy fixes, and up to you whether you make
the remaining suggested changes now or leave them for followups.

-- 
Eric Blake   eblake at redhat.com    +1-801-349-2682
Libvirt virtualization library http://libvirt.org

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 619 bytes
Desc: OpenPGP digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20110628/9526c9f6/attachment-0001.sig>