[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [libvirt] [PATCH 07/14] Implement dispatch functions for lock protocol in virtlockd



On 11.12.2012 21:41, Daniel P. Berrange wrote:
> From: "Daniel P. Berrange" <berrange redhat com>
> 
> Introduce a lock_daemon_dispatch.c file which implements the
> server side dispatcher the RPC APIs previously defined in the
> lock protocol.
> 
> Signed-off-by: Daniel P. Berrange <berrange redhat com>
> ---
>  .gitignore                         |   1 +
>  po/POTFILES.in                     |   1 +
>  src/Makefile.am                    |  14 ++
>  src/internal.h                     |  22 ++
>  src/locking/lock_daemon.c          | 133 +++++++++++-
>  src/locking/lock_daemon.h          |  13 ++
>  src/locking/lock_daemon_dispatch.c | 434 +++++++++++++++++++++++++++++++++++++
>  src/locking/lock_daemon_dispatch.h |  31 +++
>  8 files changed, 647 insertions(+), 2 deletions(-)
>  create mode 100644 src/locking/lock_daemon_dispatch.c
>  create mode 100644 src/locking/lock_daemon_dispatch.h
> 
> diff --git a/.gitignore b/.gitignore
> index 449a1c6..6de2308 100644
> --- a/.gitignore
> +++ b/.gitignore
> @@ -108,6 +108,7 @@
>  /src/libvirt_*helper
>  /src/libvirt_*probes.h
>  /src/libvirt_lxc
> +/src/locking/lock_daemon_dispatch_stubs.h
>  /src/locking/lock_protocol.[ch]
>  /src/locking/qemu-sanlock.conf
>  /src/locking/test_libvirt_sanlock.aug
> diff --git a/po/POTFILES.in b/po/POTFILES.in
> index 34c688c..c6c72a8 100644
> --- a/po/POTFILES.in
> +++ b/po/POTFILES.in
> @@ -50,6 +50,7 @@ src/libvirt.c
>  src/libvirt-qemu.c
>  src/locking/lock_daemon.c
>  src/locking/lock_daemon_config.c
> +src/locking/lock_daemon_dispatch.c
>  src/locking/lock_driver_sanlock.c
>  src/locking/lock_manager.c
>  src/locking/sanlock_helper.c
> diff --git a/src/Makefile.am b/src/Makefile.am
> index 2023f88..f0a9a03 100644
> --- a/src/Makefile.am
> +++ b/src/Makefile.am
> @@ -157,13 +157,26 @@ EXTRA_DIST += locking/lock_protocol.x
>  BUILT_SOURCES += $(LOCK_PROTOCOL_GENERATED)
>  MAINTAINERCLEANFILES += $(LOCK_PROTOCOL_GENERATED)
>  
> +LOCK_DAEMON_GENERATED = \
> +		locking/lock_daemon_dispatch_stubs.h
> +		$(NULL)
> +
> +BUILT_SOURCES += $(LOCK_DAEMON_GENERATED)
> +MAINTAINERCLEANFILES += $(LOCK_DAEMON_GENERATED)
> +
>  LOCK_DAEMON_SOURCES = \
>  		locking/lock_daemon.h \
>  		locking/lock_daemon.c \
>  		locking/lock_daemon_config.h \
>  		locking/lock_daemon_config.c \
> +		locking/lock_daemon_dispatch.c \
> +		locking/lock_daemon_dispatch.h \
>  		$(NULL)
>  
> +$(srcdir)/locking/lock_daemon_dispatch_stubs.h: locking/lock_protocol.x $(srcdir)/rpc/gendispatch.pl Makefile.am
> +	$(AM_V_GEN)perl -w $(srcdir)/rpc/gendispatch.pl -b virLockSpaceProtocol VIR_LOCK_SPACE_PROTOCOL $< > $@
> +
> +
>  NETDEV_CONF_SOURCES =						\
>  		conf/netdev_bandwidth_conf.h conf/netdev_bandwidth_conf.c \
>  		conf/netdev_vport_profile_conf.h conf/netdev_vport_profile_conf.c \
> @@ -1530,6 +1543,7 @@ sbin_PROGRAMS = virtlockd
>  virtlockd_SOURCES = \
>  		$(LOCK_DAEMON_SOURCES) \
>  		$(LOCK_PROTOCOL_GENERATED) \
> +		$(LOCK_DAEMON_GENERATED) \
>  		$(NULL)
>  virtlockd_CFLAGS = \
>  		$(AM_CFLAGS) \
> diff --git a/src/internal.h b/src/internal.h
> index d69bd14..8d96660 100644
> --- a/src/internal.h
> +++ b/src/internal.h
> @@ -236,6 +236,28 @@
>          }                                                               \
>      } while (0)
>  
> +/**
> + * virCheckFlagsGoto:
> + * @supported: an OR'ed set of supported flags
> + * @label: label to jump to on error
> + *
> + * To avoid memory leaks this macro has to be used before any non-trivial
> + * code which could possibly allocate some memory.
> + *
> + * Returns nothing. Jumps to a label if unsupported flags were
> + * passed to it.
> + */
> +# define virCheckFlagsGoto(supported, label)                            \
> +    do {                                                                \
> +        unsigned long __unsuppflags = flags & ~(supported);             \
> +        if (__unsuppflags) {                                            \
> +            virReportInvalidArg(flags,                                  \
> +                                _("unsupported flags (0x%lx) in function %s"), \
> +                                __unsuppflags, __FUNCTION__);           \
> +            goto label;                                                 \
> +        }                                                               \
> +    } while (0)
> +
>  # define virCheckNonNullArgReturn(argname, retval)  \
>      do {                                            \
>          if (argname == NULL) {                      \
> diff --git a/src/locking/lock_daemon.c b/src/locking/lock_daemon.c
> index f821d2e..3c007f1 100644
> --- a/src/locking/lock_daemon.c
> +++ b/src/locking/lock_daemon.c
> @@ -36,6 +36,7 @@
>  #include "util.h"
>  #include "virfile.h"
>  #include "virpidfile.h"
> +#include "virprocess.h"
>  #include "virterror_internal.h"
>  #include "logging.h"
>  #include "memory.h"
> @@ -44,13 +45,20 @@
>  #include "virrandom.h"
>  #include "virhash.h"
>  
> +#include "locking/lock_daemon_dispatch.h"
> +#include "locking/lock_protocol.h"
> +
>  #include "configmake.h"
>  
>  #define VIR_FROM_THIS VIR_FROM_LOCKING
>  
> +#define VIR_LOCK_DAEMON_NUM_LOCKSPACES 3
> +
>  struct _virLockDaemon {
>      virMutex lock;
>      virNetServerPtr srv;
> +    virHashTablePtr lockspaces;
> +    virLockSpacePtr defaultLockspace;
>  };
>  
>  virLockDaemonPtr lockDaemon = NULL;
> @@ -94,11 +102,19 @@ virLockDaemonFree(virLockDaemonPtr lockd)
>          return;
>  
>      virObjectUnref(lockd->srv);
> +    virHashFree(lockd->lockspaces);
> +    virLockSpaceFree(lockd->defaultLockspace);
>  
>      VIR_FREE(lockd);
>  }
>  
>  
> +static void virLockDaemonLockSpaceDataFree(void *data,
> +                                           const void *key ATTRIBUTE_UNUSED)
> +{
> +    virLockSpaceFree(data);
> +}
> +
>  static virLockDaemonPtr
>  virLockDaemonNew(bool privileged)
>  {
> @@ -125,6 +141,13 @@ virLockDaemonNew(bool privileged)
>                                         (void*)(intptr_t)(privileged ? 0x1 : 0x0))))
>          goto error;
>  
> +    if (!(lockd->lockspaces = virHashCreate(VIR_LOCK_DAEMON_NUM_LOCKSPACES,
> +                                            virLockDaemonLockSpaceDataFree)))
> +        goto error;
> +
> +    if (!(lockd->defaultLockspace = virLockSpaceNew(NULL)))
> +        goto error;
> +
>      return lockd;
>  
>  error:
> @@ -133,6 +156,31 @@ error:
>  }
>  
>  
> +int virLockDaemonAddLockSpace(virLockDaemonPtr lockd,
> +                              const char *path,
> +                              virLockSpacePtr lockspace)
> +{
> +    int ret;
> +    virMutexLock(&lockd->lock);
> +    ret = virHashAddEntry(lockd->lockspaces, path, lockspace);
> +    virMutexUnlock(&lockd->lock);
> +    return ret;
> +}
> +
> +virLockSpacePtr virLockDaemonFindLockSpace(virLockDaemonPtr lockd,
> +                                           const char *path)
> +{
> +    virLockSpacePtr lockspace;
> +    virMutexLock(&lockd->lock);
> +    if (path && STRNEQ(path, ""))
> +        lockspace = virHashLookup(lockd->lockspaces, path);
> +    else
> +        lockspace = lockd->defaultLockspace;
> +    virMutexUnlock(&lockd->lock);
> +    return lockspace;
> +}
> +
> +
>  static int
>  virLockDaemonForkIntoBackground(const char *argv0)
>  {
> @@ -466,6 +514,30 @@ virLockDaemonSetupNetworking(virNetServerPtr srv, const char *sock_path)
>  }
>  
>  
> +struct virLockDaemonClientReleaseData {
> +    virLockDaemonClientPtr client;
> +    bool hadSomeLeases;
> +    bool gotError;
> +};
> +
> +static void
> +virLockDaemonClientReleaseLockspace(void *payload,
> +                                    const void *name ATTRIBUTE_UNUSED,
> +                                    void *opaque)
> +{
> +    virLockSpacePtr lockspace = payload;
> +    struct virLockDaemonClientReleaseData *data = opaque;
> +    int rc;
> +
> +    rc = virLockSpaceReleaseResourcesForOwner(lockspace,
> +                                              data->client->clientPid);
> +    if (rc > 0)
> +        data->hadSomeLeases = true;
> +    else if (rc < 0)
> +        data->gotError = true;
> +}
> +
> +
>  static void
>  virLockDaemonClientFree(void *opaque)
>  {
> @@ -474,9 +546,52 @@ virLockDaemonClientFree(void *opaque)
>      if (!priv)
>          return;
>  
> -    VIR_DEBUG("priv=%p client=%lld",
> +    VIR_DEBUG("priv=%p client=%lld owner=%lld",
>                priv,
> -              (unsigned long long)priv->clientPid);
> +              (unsigned long long)priv->clientPid,
> +              (unsigned long long)priv->ownerPid);
> +
> +    /* If client & owner match, this is the lock holder */
> +    if (priv->clientPid == priv->ownerPid) {
> +        size_t i;
> +        struct virLockDaemonClientReleaseData data = {
> +            .client = priv, .hadSomeLeases = false, .gotError = false
> +        };
> +
> +        /* Release all locks associated with this
> +         * owner in all lockspaces */
> +        virMutexLock(&lockDaemon->lock);
> +        virHashForEach(lockDaemon->lockspaces,
> +                       virLockDaemonClientReleaseLockspace,
> +                       &data);
> +        virLockDaemonClientReleaseLockspace(lockDaemon->defaultLockspace,
> +                                            "",
> +                                            &data);
> +        virMutexUnlock(&lockDaemon->lock);
> +
> +        /* If the client had some active leases when it
> +         * closed the connection, we must kill it off
> +         * to make sure it doesn't do nasty stuff */
> +        if (data.gotError || data.hadSomeLeases) {
> +            for (i = 0 ; i < 15 ; i++) {
> +                int signum;
> +                if (i == 0)
> +                    signum = SIGTERM;
> +                else if (i == 8)
> +                    signum = SIGKILL;
> +                else
> +                    signum = 0;
> +                if (virProcessKill(priv->clientPid, signum) < 0) {
> +                    if (errno == ESRCH)
> +                        break;
> +
> +                    VIR_WARN("Failed to kill off pid %lld",
> +                             (unsigned long long)priv->clientPid);
> +                }
> +                usleep(200 * 1000);
> +            }
> +        }
> +    }
>  
>      virMutexDestroy(&priv->lock);
>      VIR_FREE(priv);
> @@ -598,6 +713,7 @@ enum {
>  
>  #define MAX_LISTEN 5
>  int main(int argc, char **argv) {
> +    virNetServerProgramPtr lockProgram = NULL;
>      char *remote_config_file = NULL;
>      int statuswrite = -1;
>      int ret = 1;
> @@ -807,6 +923,18 @@ int main(int argc, char **argv) {
>          goto cleanup;
>      }
>  
> +    if (!(lockProgram = virNetServerProgramNew(VIR_LOCK_SPACE_PROTOCOL_PROGRAM,
> +                                               VIR_LOCK_SPACE_PROTOCOL_PROGRAM_VERSION,
> +                                               virLockSpaceProtocolProcs,
> +                                               virLockSpaceProtocolNProcs))) {
> +        ret = VIR_LOCK_DAEMON_ERR_INIT;
> +        goto cleanup;
> +    }
> +    if (virNetServerAddProgram(lockDaemon->srv, lockProgram) < 0) {
> +        ret = VIR_LOCK_DAEMON_ERR_INIT;
> +        goto cleanup;
> +    }
> +
>      /* Disable error func, now logging is setup */
>      virSetErrorFunc(NULL, virLockDaemonErrorHandler);
>  
> @@ -830,6 +958,7 @@ int main(int argc, char **argv) {
>      ret = 0;
>  
>  cleanup:
> +    virObjectUnref(lockProgram);
>      virLockDaemonFree(lockDaemon);
>      if (statuswrite != -1) {
>          if (ret != 0) {
> diff --git a/src/locking/lock_daemon.h b/src/locking/lock_daemon.h
> index 7bc8c2e..619f8f2 100644
> --- a/src/locking/lock_daemon.h
> +++ b/src/locking/lock_daemon.h
> @@ -34,10 +34,23 @@ typedef virLockDaemonClient *virLockDaemonClientPtr;
>  
>  struct _virLockDaemonClient {
>      virMutex lock;
> +    bool restricted;
> +
> +    pid_t ownerPid;
> +    char *ownerName;
> +    unsigned char ownerUUID[VIR_UUID_BUFLEN];
> +    unsigned int ownerId;
>  
>      pid_t clientPid;
>  };
>  
>  extern virLockDaemonPtr lockDaemon;
>  
> +int virLockDaemonAddLockSpace(virLockDaemonPtr lockd,
> +                              const char *path,
> +                              virLockSpacePtr lockspace);
> +
> +virLockSpacePtr virLockDaemonFindLockSpace(virLockDaemonPtr lockd,
> +                                           const char *path);
> +
>  #endif /* __VIR_LOCK_DAEMON_H__ */
> diff --git a/src/locking/lock_daemon_dispatch.c b/src/locking/lock_daemon_dispatch.c
> new file mode 100644
> index 0000000..f4797a8
> --- /dev/null
> +++ b/src/locking/lock_daemon_dispatch.c
> @@ -0,0 +1,434 @@
> +/*
> + * lock_daemon_dispatch.c: lock management daemon dispatch
> + *
> + * Copyright (C) 2006-2012 Red Hat, Inc.
> + *
> + * This library is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU Lesser General Public
> + * License as published by the Free Software Foundation; either
> + * version 2.1 of the License, or (at your option) any later version.
> + *
> + * This library is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> + * Lesser General Public License for more details.
> + *
> + * You should have received a copy of the GNU Lesser General Public
> + * License along with this library;  If not, see
> + * <http://www.gnu.org/licenses/>.
> + *
> + * Author: Daniel P. Berrange <berrange redhat com>
> + */
> +
> +#include <config.h>
> +
> +#include "rpc/virnetserver.h"
> +#include "rpc/virnetserverclient.h"
> +#include "util.h"
> +#include "logging.h"
> +
> +#include "lock_daemon.h"
> +#include "lock_protocol.h"
> +#include "lock_daemon_dispatch_stubs.h"
> +#include "virterror_internal.h"
> +
> +#define VIR_FROM_THIS VIR_FROM_RPC
> +
> +static int
> +virLockSpaceProtocolDispatchAcquireResource(virNetServerPtr server ATTRIBUTE_UNUSED,
> +                                            virNetServerClientPtr client,
> +                                            virNetMessagePtr msg ATTRIBUTE_UNUSED,
> +                                            virNetMessageErrorPtr rerr,
> +                                            virLockSpaceProtocolAcquireResourceArgs *args)
> +{
> +    int rv = -1;
> +    unsigned int flags = args->flags;
> +    virLockDaemonClientPtr priv =
> +        virNetServerClientGetPrivateData(client);
> +    virLockSpacePtr lockspace;
> +    unsigned int newFlags;
> +
> +    virMutexLock(&priv->lock);
> +
> +    virCheckFlagsGoto(VIR_LOCK_SPACE_ACQUIRE_SHARED |
> +                      VIR_LOCK_SPACE_ACQUIRE_AUTOCREATE, cleanup);

se here you are effectively checking 'flags' to
VIR_LOCK_SPACE_ACQUIRE_SHARED and VIR_LOCK_SPACE_ACQUIRE_AUTOCREATE ...

> +
> +    if (priv->restricted) {
> +        virReportError(VIR_ERR_OPERATION_DENIED, "%s",
> +                       _("lock manager connection has been restricted"));
> +        goto cleanup;
> +    }
> +
> +    if (!priv->ownerPid) {
> +        virReportError(VIR_ERR_OPERATION_INVALID, "%s",
> +                       _("lock owner details have not been registered"));
> +        goto cleanup;
> +    }
> +
> +    if (!(lockspace = virLockDaemonFindLockSpace(lockDaemon, args->path))) {
> +        virReportError(VIR_ERR_INTERNAL_ERROR,
> +                       _("Lockspace for path %s does not exist"),
> +                       args->path);
> +        goto cleanup;
> +    }
> +
> +    newFlags = 0;
> +    if (flags & VIR_LOCK_SPACE_PROTOCOL_ACQUIRE_RESOURCE_SHARED)
> +        newFlags |= VIR_LOCK_SPACE_ACQUIRE_SHARED;
> +    if (flags & VIR_LOCK_SPACE_PROTOCOL_ACQUIRE_RESOURCE_AUTOCREATE)
> +        newFlags |= VIR_LOCK_SPACE_ACQUIRE_AUTOCREATE;

while here you test them to a different set of flags. I understand you
want to do translation from protocol representation of flags to API one
since they may (but shouldn't) diverge. You need to update the
virCheckFlagsGoto() then.

> +
> +    if (virLockSpaceAcquireResource(lockspace,
> +                                    args->name,
> +                                    priv->ownerPid,
> +                                    newFlags) < 0) {
> +        VIR_ERROR("FAILED");
> +        goto cleanup;
> +    }
> +
> +        VIR_ERROR("OK");

indentation

Otherwise I haven't spotted anything wrong. ACK

Michal


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]