5:31 p.m.
On 08/09/2012 09:20 AM, Daniel P. Berrange wrote:
From: "Daniel P. Berrange" <berrange(a)redhat.com>
The previously introduced virFile{Lock,Unlock} APIs provide a
way to acquire/release fcntl() locks on individual files. For
unknown reason though, the POSIX spec says that fcntl() locks
are released when *any* file handle refering to the same path
s/refering/referring/
is closed. In the following sequence
threadA: fd1 = open("foo")
threadB: fd2 = open("foo")
threadA: virFileLock(fd1)
threadB: virFileLock(fd2)
threadB: close(fd2)
you'd expect threadA to come out holding a lock on 'foo', and
indeed it does hold a lock for a very short time. Unfortunately
when threadB does close(fd2) this releases the lock associated
with fd1. For the current libvirt use case for virFileLock -
pidfiles - this doesn't matter since the lock is acquired
at startup while single threaded an never released until
exit.
To provide a more generally useful API though, it is neccessary
s/neccessary/necessary/
to introduce a slightly higher level abstraction, which is to
be referred to as a "lockspace". This is to be provided by
a virLockSpacePtr object in src/util/virlockspace.{c,h}. The
core idea is that the lockspace keeps track of what files are
already open+locked. This means that when a 2nd thread comes
along and tries to acquire a lock, it doesn't end up opening
and closing a new FD. The lockspace just checks the current
list of held locks and immediately returns VIR_ERR_RESOURCE_BUSY.
NB, the API as it stands is designed on the basis that the
files being locked are not being otherwise opened and used
by the application code. One approach to using this API is to
acquire locks based on a hash of the filepath.
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).
It is also possible to do locks directly on resources by
using a NULL lockspace directory and then using the file
path as the lock name eg
virLockSpacePtr lockspace = virLockSpaceNew(NULL)
virLockSpaceAcquireLock(lockspace, "/var/lib/libvirt/images/foo.img")
more missing semicolons
This is only safe todo though if no other part of the proces
s/todo/to do/; s/proces/process/
will be opening the files. This will be the case when this
code is used inside the soon-to-be-reposted virlockd daemon
Signed-off-by: Daniel P. Berrange <berrange(a)redhat.com>
---
This patch failed to apply directly for me; you'll have to rebase it
again. I was able to figure out the changes, though (a new error in
virterror.[ch]).
+
+
+static char *virLockSpaceGetResourcePath(virLockSpacePtr lockspace,
+ const char *resname)
+{
+ char *ret;
+ if (lockspace->dir) {
+ if (virAsprintf(&ret, "%s/%s", lockspace->dir, resname) < 0)
{
+ virReportOOMError();
+ return NULL;
+ }
+ } else {
+ if (!(ret = strdup(resname))) {
+ virReportOOMError();
+ return NULL;
+ }
+ }
Fine as-is, but could be written shorter as:
if (lockspace->dir)
ignore_value(virAsprintf(&ret, "%s/%s", lockspace->dir, resname));
else
ret = strdup(resname);
if (!ret) {
virReportOOMError();
return NULL;
}
+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()?
+static virLockSpaceResourcePtr
+virLockSpaceResourceNew(virLockSpacePtr lockspace,
+ VIR_DEBUG("Resource '%s' disappeared:
%s",
+ res->path, virStrerror(errno, ebuf, sizeof(ebuf)));
+ VIR_FORCE_CLOSE(res->fd);
+ /* Someone else must be racing with us, so try agin */
s/agin/again/
+
+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?
+++ b/tests/virlockspacetest.c
+
+static int testLockSpaceCreate(const void *args ATTRIBUTE_UNUSED)
+{
+ virLockSpacePtr lockspace;
+ int ret = -1;
+
+ rmdir(LOCKSPACE_DIR);
No checks for errors?
+
+ lockspace = virLockSpaceNew(LOCKSPACE_DIR);
+
+ if (!virFileIsDir(LOCKSPACE_DIR))
+ goto cleanup;
Although this will still fail if we ignored an earlier failure to
rmdir() a non-directory, so I guess you're okay for test purposes.
ACK with nits addressed.
--
Eric Blake eblake(a)redhat.com +1-919-301-3266
Libvirt virtualization library http://libvirt.org