[libvirt] [PATCH 1/3] locking: relax PID requirement

[Daniel P. Berrange]

On Fri, Apr 17, 2015 at 03:36:20PM -0600, Jim Fehlig wrote:
> Some hypervisors like Xen do not have PIDs associated with domains.
> Relax the requirement for PID != 0 in the locking code so it can
> be used by hypervisors that do not represent domains as a process
> running on the host.
> 
> Signed-off-by: Jim Fehlig <jfehlig at suse.com>
> ---
>  src/locking/lock_daemon.c          |  2 +-
>  src/locking/lock_daemon_dispatch.c | 49 +++++++++++---------------------------
>  src/locking/lock_driver_lockd.c    |  7 ++----
>  3 files changed, 17 insertions(+), 41 deletions(-)
> 
> diff --git a/src/locking/lock_daemon.c b/src/locking/lock_daemon.c
> index bb165c0..042ff94 100644
> --- a/src/locking/lock_daemon.c
> +++ b/src/locking/lock_daemon.c
> @@ -678,7 +678,7 @@ virLockDaemonClientFree(void *opaque)
>                      signum = SIGKILL;
>                  else
>                      signum = 0;
> -                if (virProcessKill(priv->clientPid, signum) < 0) {
> +                if (priv->clientPid != 0 && virProcessKill(priv->clientPid, signum) < 0) {
>                      if (errno == ESRCH)
>                          break;
>  
> diff --git a/src/locking/lock_daemon_dispatch.c b/src/locking/lock_daemon_dispatch.c
> index a7cee9d..f2704ec 100644
> --- a/src/locking/lock_daemon_dispatch.c
> +++ b/src/locking/lock_daemon_dispatch.c
> @@ -62,11 +62,8 @@ virLockSpaceProtocolDispatchAcquireResource(virNetServerPtr server ATTRIBUTE_UNU
>          goto cleanup;
>      }
>  
> -    if (!priv->ownerPid) {
> -        virReportError(VIR_ERR_OPERATION_INVALID, "%s",
> -                       _("lock owner details have not been registered"));
> -        goto cleanup;
> -    }
> +    if (!priv->ownerPid)
> +        VIR_WARN("lock owner PID has not been registered");

In this file, all these ownerPid checks are really there to validate that
the virLockSpaceProtocolDispatchRegister() method has been called first.

I think we need to check that check, so rather than removing all these
error reports, lets change them all to check priv->ownerId instead of
priv->ownerPid.

>  
>      if (!(lockspace = virLockDaemonFindLockSpace(lockDaemon, args->path))) {
>          virReportError(VIR_ERR_INTERNAL_ERROR,
> @@ -120,11 +117,8 @@ virLockSpaceProtocolDispatchCreateResource(virNetServerPtr server ATTRIBUTE_UNUS
>          goto cleanup;
>      }
>  
> -    if (!priv->ownerPid) {
> -        virReportError(VIR_ERR_OPERATION_INVALID, "%s",
> -                       _("lock owner details have not been registered"));
> -        goto cleanup;
> -    }
> +    if (!priv->ownerPid)
> +        VIR_WARN("lock owner PID has not been registered");
>  
>      if (!(lockspace = virLockDaemonFindLockSpace(lockDaemon, args->path))) {
>          virReportError(VIR_ERR_INTERNAL_ERROR,
> @@ -169,11 +163,8 @@ virLockSpaceProtocolDispatchDeleteResource(virNetServerPtr server ATTRIBUTE_UNUS
>          goto cleanup;
>      }
>  
> -    if (!priv->ownerPid) {
> -        virReportError(VIR_ERR_OPERATION_INVALID, "%s",
> -                       _("lock owner details have not been registered"));
> -        goto cleanup;
> -    }
> +    if (!priv->ownerPid)
> +        VIR_WARN("lock owner PID has not been registered");
>  
>      if (!(lockspace = virLockDaemonFindLockSpace(lockDaemon, args->path))) {
>          virReportError(VIR_ERR_INTERNAL_ERROR,
> @@ -218,11 +209,8 @@ virLockSpaceProtocolDispatchNew(virNetServerPtr server ATTRIBUTE_UNUSED,
>          goto cleanup;
>      }
>  
> -    if (!priv->ownerPid) {
> -        virReportError(VIR_ERR_OPERATION_INVALID, "%s",
> -                       _("lock owner details have not been registered"));
> -        goto cleanup;
> -    }
> +    if (!priv->ownerPid)
> +        VIR_WARN("lock owner PID has not been registered");
>  
>      if (!args->path || STREQ(args->path, "")) {
>          virReportError(VIR_ERR_OPERATION_INVALID, "%s",
> @@ -273,11 +261,8 @@ virLockSpaceProtocolDispatchRegister(virNetServerPtr server ATTRIBUTE_UNUSED,
>          goto cleanup;
>      }
>  
> -    if (priv->ownerPid) {
> -        virReportError(VIR_ERR_OPERATION_INVALID, "%s",
> -                       _("lock owner details have already been registered"));
> -        goto cleanup;
> -    }
> +    if (priv->ownerPid)
> +        VIR_WARN("lock owner PID has not been registered");
>  
>      if (VIR_STRDUP(priv->ownerName, args->owner.name) < 0)
>          goto cleanup;
> @@ -320,11 +305,8 @@ virLockSpaceProtocolDispatchReleaseResource(virNetServerPtr server ATTRIBUTE_UNU
>          goto cleanup;
>      }
>  
> -    if (!priv->ownerPid) {
> -        virReportError(VIR_ERR_OPERATION_INVALID, "%s",
> -                       _("lock owner details have not been registered"));
> -        goto cleanup;
> -    }
> +    if (!priv->ownerPid)
> +        VIR_WARN("lock owner PID has not been registered");
>  
>      if (!(lockspace = virLockDaemonFindLockSpace(lockDaemon, args->path))) {
>          virReportError(VIR_ERR_INTERNAL_ERROR,
> @@ -370,11 +352,8 @@ virLockSpaceProtocolDispatchRestrict(virNetServerPtr server ATTRIBUTE_UNUSED,
>          goto cleanup;
>      }
>  
> -    if (!priv->ownerPid) {
> -        virReportError(VIR_ERR_OPERATION_INVALID, "%s",
> -                       _("lock owner details have not been registered"));
> -        goto cleanup;
> -    }
> +    if (!priv->ownerPid)
> +        VIR_WARN("lock owner PID has not been registered");
>  
>      priv->restricted = true;
>      rv = 0;
> diff --git a/src/locking/lock_driver_lockd.c b/src/locking/lock_driver_lockd.c
> index 3a48a6a..109a79b 100644
> --- a/src/locking/lock_driver_lockd.c
> +++ b/src/locking/lock_driver_lockd.c
> @@ -466,11 +466,8 @@ static int virLockManagerLockDaemonNew(virLockManagerPtr lock,
>                             _("Missing ID parameter for domain object"));
>              return -1;
>          }
> -        if (priv->pid == 0) {
> -            virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> -                           _("Missing PID parameter for domain object"));
> -            return -1;
> -        }
> +        if (priv->pid == 0)
> +            VIR_WARN("Missing PID parameter for domain object");

Lets just make that a VIR_DEBUG, since you don't want the log polluted
with warnings every time a xen guest is started.

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