On 08/09/2012 09:20 AM, Daniel P. Berrange wrote: > From: "Daniel P. Berrange" <berra...@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 <berra...@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 ebl...@redhat.com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
signature.asc
Description: OpenPGP digital signature
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
