[libvirt] [PATCH 2/2] sanlock: don't fail with unregistered domains

[John Ferlan]

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 at 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,
>