[libvirt] [PATCH 19/23] Implement dispatch functions for lock protocol in virtlockd

[Eric Blake]

On 08/09/2012 09:20 AM, Daniel P. Berrange wrote:
> From: "Daniel P. Berrange" <berrange at 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 at redhat.com>
> ---
>  .gitignore                         |   1 +
>  po/POTFILES.in                     |   1 +
>  src/Makefile.am                    |  14 ++
>  src/internal.h                     |  22 +++
>  src/locking/lock_daemon.c          | 130 ++++++++++++-
>  src/locking/lock_daemon.h          |  13 ++
>  src/locking/lock_daemon_dispatch.c | 370 +++++++++++++++++++++++++++++++++++++
>  src/locking/lock_daemon_dispatch.h |  31 ++++
>  8 files changed, 580 insertions(+), 2 deletions(-)
>  create mode 100644 src/locking/lock_daemon_dispatch.c
>  create mode 100644 src/locking/lock_daemon_dispatch.h

I hit a merge conflict applying this one, as well (this time in
src/Makefile.am); shouldn't be too hard to rebase.

> +++ b/src/Makefile.am
> @@ -149,11 +149,24 @@ 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)

Missing a \

>  
> +$(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 $< > $@

Long lines; some \ would be nice.

> +++ b/src/internal.h
> @@ -240,6 +240,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.

Stale comment - the goto is what lets you jump to an error label, and
thus clean up any non-trivial code used before this macro.

> +        /* 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;

Rather than open-coding this loop, can't you use virPidAbort() from
util/command.h?

> +++ b/src/locking/lock_daemon_dispatch.c

> +
> +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;

If you check flags here...

> +    virLockDaemonClientPtr priv =
> +        virNetServerClientGetPrivateData(client);

...or delay the assignment of priv...

> +    virLockSpacePtr lockspace;
> +    unsigned int newFlags;

...and check flags here, with virCheckFlags(..., -1)...

> +
> +    virMutexLock(&priv->lock);
> +
> +    virCheckFlagsGoto(VIR_LOCK_SPACE_ACQUIRE_SHARED |
> +                      VIR_LOCK_SPACE_ACQUIRE_AUTOCREATE, cleanup);

...then you wouldn't need virCheckFlagsGoto.  Then again, I don't mind
having the new macro, as it gives you the option to put flag checks at a
location easier to read.

> +
> +    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;

Any reason you have two namespaces of flags, and have to translate
between them, rather than guaranteeing that the flags have the same
values and can be reused in both contexts?

-- 
Eric Blake   eblake at redhat.com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 620 bytes
Desc: OpenPGP digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20120817/0ef5ca0b/attachment-0001.sig>