[libvirt] [PATCH 11/23] Introduce an internal API for handling file based lockspaces

[Daniel P. Berrange]

On Thu, Aug 16, 2012 at 04:31:28PM -0600, Eric Blake wrote:
> On 08/09/2012 09:20 AM, Daniel P. Berrange wrote:
> > eg to lock /var/lib/libvirt/images/foo.img the application
> > might do
> > 
> >    virLockSpacePtr lockspace = virLockSpaceNew("/var/lib/libvirt/imagelocks");
> >    lockname = md5sum("/var/lib/libvirt/images/foo.img")
> >    virLockSpaceAcquireLock(lockspace, lockname)
> 
> s/)$/),/g
> 
> Don't you want to ensure that the canonical name is used (that is,
> symlinks can allow two different names to resolve to the same file, but
> you want to hash the name without symlinks).

Yes, I added a note that the filename should be canonicalized
in this example

> > +static void virLockSpaceResourceFree(virLockSpaceResourcePtr res)
> > +{
> > +    if (!res)
> > +        return;
> > +
> > +    if (res->lockHeld &&
> > +        (res->flags & VIR_LOCK_SPACE_ACQUIRE_AUTOCREATE)) {
> > +        if (res->flags & VIR_LOCK_SPACE_ACQUIRE_SHARED) {
> > +            /* We must upgrade to an exclusive lock to ensure
> > +             * no one else still has it before trying to delete */
> > +            if (virFileLock(res->fd, false, 0, 1) < 0) {
> > +                VIR_DEBUG("Could not upgrade shared lease to exclusive, not deleting");
> > +            } else {
> > +                unlink(res->path);
> 
> Should we log failures to unlink()?

Yeah, worth while, so I changed it to

                if (unlink(res->path) < 0 &&
                    errno != ENOENT) {
                    char ebuf[1024];
                    VIR_WARN("Failed to unlink resource %s: %s",
                             res->path, virStrerror(ioError, ebuf, sizeof(ebuf)));
                }

> > +
> > +void virLockSpaceFree(virLockSpacePtr lockspace)
> > +{
> > +    if (!lockspace)
> > +        return;
> > +
> > +    virHashFree(lockspace->resources);
> > +    VIR_FREE(lockspace->dir);
> 
> Should we first try to rmdir() the directory, and silently ignore errors
> related to the directory still being full?

I think it is best to just leave it alone. In most production
setups I expect this directory would in fact be as NFS mount.



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