8:37 a.m.
Just move some code around for future patches to ease the review.
With this patch there is no need for drastic cleanup path later.
Signed-off-by: Martin Kletzander <mkletzan(a)redhat.com>
---
src/locking/lock_driver_sanlock.c | 50 +++++++++++++++++++--------------------
1 file changed, 25 insertions(+), 25 deletions(-)
diff --git a/src/locking/lock_driver_sanlock.c b/src/locking/lock_driver_sanlock.c
index 7e4258d..01441a0 100644
--- a/src/locking/lock_driver_sanlock.c
+++ b/src/locking/lock_driver_sanlock.c
@@ -896,31 +896,6 @@ static int virLockManagerSanlockAcquire(virLockManagerPtr lock,
if (VIR_ALLOC(opt) < 0)
return -1;
- /* sanlock doesn't use owner_name for anything, so it's safe to take just
- * the first SANLK_NAME_LEN - 1 characters from vm_name */
- ignore_value(virStrncpy(opt->owner_name, priv->vm_name,
- SANLK_NAME_LEN - 1, SANLK_NAME_LEN));
-
- if (state && STRNEQ(state, "")) {
- if ((rv = sanlock_state_to_args((char *)state,
- &res_count,
- &res_args)) < 0) {
- if (rv <= -200)
- virReportError(VIR_ERR_INTERNAL_ERROR,
- _("Unable to parse lock state %s: error %d"),
- state, rv);
- else
- virReportSystemError(-rv,
- _("Unable to parse lock state %s"),
- state);
- goto error;
- }
- res_free = true;
- } else {
- res_args = priv->res_args;
- res_count = priv->res_count;
- }
-
/* We only initialize 'sock' if we are in the real
* child process and we need it to be inherited
*
@@ -949,6 +924,31 @@ static int virLockManagerSanlockAcquire(virLockManagerPtr lock,
}
}
+ /* sanlock doesn't use owner_name for anything, so it's safe to take just
+ * the first SANLK_NAME_LEN - 1 characters from vm_name */
+ ignore_value(virStrncpy(opt->owner_name, priv->vm_name,
+ SANLK_NAME_LEN - 1, SANLK_NAME_LEN));
+
+ if (state && STRNEQ(state, "")) {
+ if ((rv = sanlock_state_to_args((char *)state,
+ &res_count,
+ &res_args)) < 0) {
+ if (rv <= -200)
+ virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("Unable to parse lock state %s: error %d"),
+ state, rv);
+ else
+ virReportSystemError(-rv,
+ _("Unable to parse lock state %s"),
+ state);
+ goto error;
+ }
+ res_free = true;
+ } else {
+ res_args = priv->res_args;
+ res_count = priv->res_count;
+ }
+
if (!(flags & VIR_LOCK_MANAGER_ACQUIRE_REGISTER_ONLY)) {
VIR_DEBUG("Acquiring object %u", priv->res_count);
if ((rv = sanlock_acquire(sock, priv->vm_pid, 0,
--
1.9.2
8:37 a.m.
New subject: [libvirt] [PATCH 2/2] sanlock: don't fail with unregistered domains
When a domain was started without registration in sanlock, but libvirt
was restarted after that, most of the operations failed due to
contacting sanlock about that process. E.g. migration could not be
performed because the locks couldn't be released (or inquired before a
release).
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1088034
Signed-off-by: Martin Kletzander <mkletzan(a)redhat.com>
---
src/locking/lock_driver_sanlock.c | 31 +++++++++++++++++++++++++++++++
1 file changed, 31 insertions(+)
diff --git a/src/locking/lock_driver_sanlock.c b/src/locking/lock_driver_sanlock.c
index 01441a0..5bc72ba 100644
--- a/src/locking/lock_driver_sanlock.c
+++ b/src/locking/lock_driver_sanlock.c
@@ -91,6 +91,9 @@ struct _virLockManagerSanlockPrivate {
bool hasRWDisks;
int res_count;
struct sanlk_resource *res_args[SANLK_MAX_RESOURCES];
+
+ /* whether the VM was registered or not */
+ bool registered;
};
/*
@@ -450,6 +453,7 @@ static int virLockManagerSanlockNew(virLockManagerPtr lock,
virLockManagerParamPtr param;
virLockManagerSanlockPrivatePtr priv;
size_t i;
+ int resCount = 0;
virCheckFlags(0, -1);
@@ -487,6 +491,16 @@ static int virLockManagerSanlockNew(virLockManagerPtr lock,
}
}
+ /* Sanlock needs process registration, but the only way how to probe
+ * whether a process has been registered is ti inquire the lock. If
+ * sanlock_inquire() returns -ESRCH, then it is not registered, but
+ * if it returns any other error (rv < 0), then we cannot fail due
+ * to back-compat. So this whole call is non-fatal, because it's
+ * called from all over the place (is will usually fail). It merely
+ * updates privateData. */
+ if (sanlock_inquire(-1, priv->vm_pid, 0, &resCount, NULL) >= 0)
+ priv->registered = true;
+
lock->privateData = priv;
return 0;
@@ -915,6 +929,9 @@ static int virLockManagerSanlockAcquire(virLockManagerPtr lock,
goto error;
}
+ /* Mark the pid as registered */
+ priv->registered = true;
+
if (action != VIR_DOMAIN_LOCK_FAILURE_DEFAULT) {
char uuidstr[VIR_UUID_STRING_BUFLEN];
virUUIDFormat(priv->vm_uuid, uuidstr);
@@ -922,6 +939,9 @@ static int virLockManagerSanlockAcquire(virLockManagerPtr lock,
uuidstr, action) < 0)
goto error;
}
+ } else if (!priv->registered) {
+ VIR_DEBUG("Process not registered, not acquiring lock");
+ return 0;
}
/* sanlock doesn't use owner_name for anything, so it's safe to take just
@@ -1025,6 +1045,11 @@ static int virLockManagerSanlockRelease(virLockManagerPtr lock,
virCheckFlags(0, -1);
+ if (!priv->registered) {
+ VIR_DEBUG("Process not registered, skipping release");
+ return 0;
+ }
+
if (state) {
if ((rv = sanlock_inquire(-1, priv->vm_pid, 0, &res_count, state)) < 0)
{
if (rv <= -200)
@@ -1070,6 +1095,12 @@ static int virLockManagerSanlockInquire(virLockManagerPtr lock,
VIR_DEBUG("pid=%d", priv->vm_pid);
+ if (!priv->registered) {
+ VIR_DEBUG("Process not registered, skipping inquiry");
+ VIR_FREE(*state);
+ return 0;
+ }
+
if ((rv = sanlock_inquire(-1, priv->vm_pid, 0, &res_count, state)) < 0) {
if (rv <= -200)
virReportError(VIR_ERR_INTERNAL_ERROR,
--
1.9.2
3:13 p.m.
New subject: [libvirt] [PATCH 2/2] sanlock: don't fail with unregistered domains
On 05/12/2014 07:37 AM, Martin Kletzander wrote:
When a domain was started without registration in sanlock, but
libvirt
was restarted after that, most of the operations failed due to
contacting sanlock about that process. E.g. migration could not be
performed because the locks couldn't be released (or inquired before a
release).
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1088034
Signed-off-by: Martin Kletzander <mkletzan(a)redhat.com>
---
src/locking/lock_driver_sanlock.c | 31 +++++++++++++++++++++++++++++++
1 file changed, 31 insertions(+)
@@ -487,6 +491,16 @@ static int virLockManagerSanlockNew(virLockManagerPtr lock,
}
}
+ /* Sanlock needs process registration, but the only way how to probe
+ * whether a process has been registered is ti inquire the lock. If
s/ti/to/
+ * sanlock_inquire() returns -ESRCH, then it is not registered,
but
+ * if it returns any other error (rv < 0), then we cannot fail due
+ * to back-compat. So this whole call is non-fatal, because it's
+ * called from all over the place (is will usually fail). It merely
s/is/it/
ACK with typo fixes
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
6:25 a.m.
New subject: [libvirt] [PATCH 2/2] sanlock: don't fail with unregistered domains
On 05/12/2014 09:37 AM, Martin Kletzander wrote:
When a domain was started without registration in sanlock, but
libvirt
was restarted after that, most of the operations failed due to
contacting sanlock about that process. E.g. migration could not be
performed because the locks couldn't be released (or inquired before a
release).
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1088034
Signed-off-by: Martin Kletzander <mkletzan(a)redhat.com>
---
src/locking/lock_driver_sanlock.c | 31 +++++++++++++++++++++++++++++++
1 file changed, 31 insertions(+)
diff --git a/src/locking/lock_driver_sanlock.c b/src/locking/lock_driver_sanlock.c
index 01441a0..5bc72ba 100644
--- a/src/locking/lock_driver_sanlock.c
+++ b/src/locking/lock_driver_sanlock.c
@@ -91,6 +91,9 @@ struct _virLockManagerSanlockPrivate {
bool hasRWDisks;
int res_count;
struct sanlk_resource *res_args[SANLK_MAX_RESOURCES];
+
+ /* whether the VM was registered or not */
+ bool registered;
};
/*
@@ -450,6 +453,7 @@ static int virLockManagerSanlockNew(virLockManagerPtr lock,
virLockManagerParamPtr param;
virLockManagerSanlockPrivatePtr priv;
size_t i;
+ int resCount = 0;
virCheckFlags(0, -1);
@@ -487,6 +491,16 @@ static int virLockManagerSanlockNew(virLockManagerPtr lock,
}
}
+ /* Sanlock needs process registration, but the only way how to probe
+ * whether a process has been registered is ti inquire the lock. If
+ * sanlock_inquire() returns -ESRCH, then it is not registered, but
+ * if it returns any other error (rv < 0), then we cannot fail due
+ * to back-compat. So this whole call is non-fatal, because it's
+ * called from all over the place (is will usually fail). It merely
+ * updates privateData. */
+ if (sanlock_inquire(-1, priv->vm_pid, 0, &resCount, NULL) >= 0)
+ priv->registered = true;
+
lock->privateData = priv;
return 0;
@@ -915,6 +929,9 @@ static int virLockManagerSanlockAcquire(virLockManagerPtr lock,
goto error;
}
+ /* Mark the pid as registered */
+ priv->registered = true;
+
if (action != VIR_DOMAIN_LOCK_FAILURE_DEFAULT) {
char uuidstr[VIR_UUID_STRING_BUFLEN];
virUUIDFormat(priv->vm_uuid, uuidstr);
@@ -922,6 +939,9 @@ static int virLockManagerSanlockAcquire(virLockManagerPtr lock,
uuidstr, action) < 0)
goto error;
}
+ } else if (!priv->registered) {
+ VIR_DEBUG("Process not registered, not acquiring lock");
+ return 0;
Coverity found an issue regarding 'opt' not being VIR_FREE()'d
John
}
/* sanlock doesn't use owner_name for anything, so it's safe to take just
@@ -1025,6 +1045,11 @@ static int virLockManagerSanlockRelease(virLockManagerPtr lock,
virCheckFlags(0, -1);
+ if (!priv->registered) {
+ VIR_DEBUG("Process not registered, skipping release");
+ return 0;
+ }
+
if (state) {
if ((rv = sanlock_inquire(-1, priv->vm_pid, 0, &res_count, state)) <
0) {
if (rv <= -200)
@@ -1070,6 +1095,12 @@ static int virLockManagerSanlockInquire(virLockManagerPtr lock,
VIR_DEBUG("pid=%d", priv->vm_pid);
+ if (!priv->registered) {
+ VIR_DEBUG("Process not registered, skipping inquiry");
+ VIR_FREE(*state);
+ return 0;
+ }
+
if ((rv = sanlock_inquire(-1, priv->vm_pid, 0, &res_count, state)) < 0) {
if (rv <= -200)
virReportError(VIR_ERR_INTERNAL_ERROR,
6:38 a.m.
New subject: [libvirt] [PATCH 2/2] sanlock: don't fail with unregistered domains
On Tue, May 13, 2014 at 07:25:29AM -0400, John Ferlan wrote:
On 05/12/2014 09:37 AM, Martin Kletzander wrote:
> When a domain was started without registration in sanlock, but libvirt
> was restarted after that, most of the operations failed due to
> contacting sanlock about that process. E.g. migration could not be
> performed because the locks couldn't be released (or inquired before a
> release).
>
> Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1088034
>
> Signed-off-by: Martin Kletzander <mkletzan(a)redhat.com>
> ---
> src/locking/lock_driver_sanlock.c | 31 +++++++++++++++++++++++++++++++
> 1 file changed, 31 insertions(+)
>
> diff --git a/src/locking/lock_driver_sanlock.c b/src/locking/lock_driver_sanlock.c
> index 01441a0..5bc72ba 100644
[...]
> @@ -915,6 +929,9 @@ static int
virLockManagerSanlockAcquire(virLockManagerPtr lock,
> goto error;
> }
>
> + /* Mark the pid as registered */
> + priv->registered = true;
> +
> if (action != VIR_DOMAIN_LOCK_FAILURE_DEFAULT) {
> char uuidstr[VIR_UUID_STRING_BUFLEN];
> virUUIDFormat(priv->vm_uuid, uuidstr);
> @@ -922,6 +939,9 @@ static int virLockManagerSanlockAcquire(virLockManagerPtr lock,
> uuidstr, action) < 0)
> goto error;
> }
> + } else if (!priv->registered) {
> + VIR_DEBUG("Process not registered, not acquiring lock");
> + return 0;
Coverity found an issue regarding 'opt' not being VIR_FREE()'d
Oh, thanks for finding that, this part should've been moved in the
previous patch as well. I'll post a follow-up fixing it since I've
pushed the patches already.
Martin
3:11 p.m.
On 05/12/2014 07:37 AM, Martin Kletzander wrote:
Just move some code around for future patches to ease the review.
With this patch there is no need for drastic cleanup path later.
Signed-off-by: Martin Kletzander <mkletzan(a)redhat.com>
---
src/locking/lock_driver_sanlock.c | 50 +++++++++++++++++++--------------------
1 file changed, 25 insertions(+), 25 deletions(-)
ACK
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
3838
days inactive
3839
days old
5 comments
3 participants
participants (3)
-
Eric Blake -
John Ferlan -
Martin Kletzander