On 09/10/2018 05:36 AM, Michal Privoznik wrote:
> Soon there will be a virtlockd client that wants to either lock
> all the resources or none (in order to avoid virtlockd killing
> the client on connection close). Because on the RPC layer we can
> only acquire one resource at a time, we have to perform a
> rollback once we hit a resource that can't be acquired.
>
> Signed-off-by: Michal Privoznik <mpriv...@redhat.com>
> ---
> src/locking/lock_driver.h | 4 ++
> src/locking/lock_driver_lockd.c | 84
> +++++++++++++++++++++++++++++------------
> 2 files changed, 64 insertions(+), 24 deletions(-)
>
> diff --git a/src/locking/lock_driver.h b/src/locking/lock_driver.h
> index 9be0abcfba..8d236471d4 100644
> --- a/src/locking/lock_driver.h
> +++ b/src/locking/lock_driver.h
> @@ -67,6 +67,10 @@ typedef enum {
> VIR_LOCK_MANAGER_ACQUIRE_REGISTER_ONLY = (1 << 0),
> /* Prevent further lock/unlock calls from this process */
> VIR_LOCK_MANAGER_ACQUIRE_RESTRICT = (1 << 1),
> + /* In case when acquiring more resources which one of them
s/In case/Used/
s/which/in which/
> + * can't be acquired, perform a rollback and release all
> + * resources acquired so far. */
> + VIR_LOCK_MANAGER_ACQUIRE_ROLLBACK = (1 << 2),
> } virLockManagerAcquireFlags;
>
> typedef enum {
> diff --git a/src/locking/lock_driver_lockd.c b/src/locking/lock_driver_lockd.c
> index cb294ac694..3068a72507 100644
> --- a/src/locking/lock_driver_lockd.c
> +++ b/src/locking/lock_driver_lockd.c
> @@ -729,6 +729,34 @@ static int
> virLockManagerLockDaemonAddResource(virLockManagerPtr lock,
> }
>
>
> +static int virLockManagerLockDaemonReleaseImpl(virNetClientPtr client,
> + virNetClientProgramPtr
> program,
> + int *counter,
> +
> virLockManagerLockDaemonResourcePtr res)
> +{
> + virLockSpaceProtocolReleaseResourceArgs args;
> +
> + memset(&args, 0, sizeof(args));
> +
> + args.path = res->lockspace;
> + args.name = res->name;
> + args.flags = res->flags;
> +
> + args.flags &=
> + ~(VIR_LOCK_SPACE_PROTOCOL_ACQUIRE_RESOURCE_SHARED |
> + VIR_LOCK_SPACE_PROTOCOL_ACQUIRE_RESOURCE_AUTOCREATE |
> + VIR_LOCK_SPACE_PROTOCOL_ACQUIRE_RESOURCE_METADATA);
> +
> + return virNetClientProgramCall(program,
> + client,
> + (*counter)++,
This sticks out like a sore thumb in the annals of C-isms for me.
Something about "when" one can assume/expect the autoincr to happen when
there's a *value involved.
Regardless, I don't think it needs to be done this way, just pass
counter and do the autoincr in the caller... see my continued comments
below [1]
> +
> VIR_LOCK_SPACE_PROTOCOL_PROC_RELEASE_RESOURCE,
> + 0, NULL, NULL, NULL,
> +
> (xdrproc_t)xdr_virLockSpaceProtocolReleaseResourceArgs, &args,
> + (xdrproc_t)xdr_void, NULL);
> +}
> +
> +
> static int virLockManagerLockDaemonAcquire(virLockManagerPtr lock,
> const char *state
> ATTRIBUTE_UNUSED,
> unsigned int flags,
> @@ -739,10 +767,13 @@ static int
> virLockManagerLockDaemonAcquire(virLockManagerPtr lock,
> virNetClientProgramPtr program = NULL;
> int counter = 0;
> int rv = -1;
> + ssize_t i;
> + ssize_t lastGood = -1;
> virLockManagerLockDaemonPrivatePtr priv = lock->privateData;
>
> virCheckFlags(VIR_LOCK_MANAGER_ACQUIRE_REGISTER_ONLY |
> - VIR_LOCK_MANAGER_ACQUIRE_RESTRICT, -1);
> + VIR_LOCK_MANAGER_ACQUIRE_RESTRICT |
> + VIR_LOCK_MANAGER_ACQUIRE_ROLLBACK, -1);
>
> if (priv->type == VIR_LOCK_MANAGER_OBJECT_TYPE_DOMAIN &&
> priv->nresources == 0 &&
> @@ -761,7 +792,6 @@ static int
> virLockManagerLockDaemonAcquire(virLockManagerPtr lock,
> goto cleanup;
>
> if (!(flags & VIR_LOCK_MANAGER_ACQUIRE_REGISTER_ONLY)) {
> - size_t i;
> for (i = 0; i < priv->nresources; i++) {
> virLockSpaceProtocolAcquireResourceArgs args;
>
> @@ -779,6 +809,7 @@ static int
> virLockManagerLockDaemonAcquire(virLockManagerPtr lock,
>
> (xdrproc_t)xdr_virLockSpaceProtocolAcquireResourceArgs, &args,
> (xdrproc_t)xdr_void, NULL) < 0)
> goto cleanup;
> + lastGood = i;
> }
> }
>
> @@ -789,8 +820,30 @@ static int
> virLockManagerLockDaemonAcquire(virLockManagerPtr lock,
> rv = 0;
>
> cleanup:
> - if (rv != 0 && fd)
> - VIR_FORCE_CLOSE(*fd);
> + if (rv < 0) {
> + int saved_errno = errno;
> + virErrorPtr origerr;
> +
> + virErrorPreserveLast(&origerr);
> + if (fd)
> + VIR_FORCE_CLOSE(*fd);
> +
> + if (client && program &&
> + flags & VIR_LOCK_MANAGER_ACQUIRE_ROLLBACK &&
> + !(flags & VIR_LOCK_MANAGER_ACQUIRE_REGISTER_ONLY)) {
Not sure any of the above 3 mean anything since lastGood is only set > 0
in one place so I would think the subsequent loop is good alone. I
haven't looked ahead though ;-)
> + for (i = lastGood; i >= 0; i--) {
> + virLockManagerLockDaemonResourcePtr res =
> &priv->resources[i];
> +
> + if (virLockManagerLockDaemonReleaseImpl(client, program,
> + &counter, res) < 0)
> + VIR_WARN("Unable to release resource lockspace=%s
> name=%s",
> + res->lockspace, res->name);
> + }
> + }
> +
> + virErrorRestore(&origerr);
> + errno = saved_errno;
> + }
> virNetClientClose(client);
> virObjectUnref(client);
> virObjectUnref(program);
> @@ -818,27 +871,10 @@ static int
> virLockManagerLockDaemonRelease(virLockManagerPtr lock,
> goto cleanup;
>
> for (i = 0; i < priv->nresources; i++) {
[1]
I think you can get away with this:
s/i++/i++, counter++/
and not pass &counter for it to be incremented in the helper. There's no
way "out of" the helper without incrementing it (thus far at least). So
since this code owns it and can increment more sanely, I'd stick with it
here.
> - virLockSpaceProtocolReleaseResourceArgs args;
> + virLockManagerLockDaemonResourcePtr res = &priv->resources[i];
>
> - memset(&args, 0, sizeof(args));
> -
> - if (priv->resources[i].lockspace)
> - args.path = priv->resources[i].lockspace;
> - args.name = priv->resources[i].name;
> - args.flags = priv->resources[i].flags;
> -
> - args.flags &=
> - ~(VIR_LOCK_SPACE_PROTOCOL_ACQUIRE_RESOURCE_SHARED |
> - VIR_LOCK_SPACE_PROTOCOL_ACQUIRE_RESOURCE_AUTOCREATE |
> - VIR_LOCK_SPACE_PROTOCOL_ACQUIRE_RESOURCE_METADATA);
> -
> - if (virNetClientProgramCall(program,
> - client,
> - counter++,
> -
> VIR_LOCK_SPACE_PROTOCOL_PROC_RELEASE_RESOURCE,
> - 0, NULL, NULL, NULL,
> -
> (xdrproc_t)xdr_virLockSpaceProtocolReleaseResourceArgs, &args,
> - (xdrproc_t)xdr_void, NULL) < 0)
> + if (virLockManagerLockDaemonReleaseImpl(client, program,
> + &counter, res) < 0)
> goto cleanup;
[1] if not in the predicate of the for loop, then right here with an
autoincr.
> }
>
>
I'm not insistent about the @lastGood stuff - go whichever way you want
in the cleanup code; however, I would much prefer the counter increment
to happen in this context not in the called helper context for the
Reviewed-by: John Ferlan <jfer...@redhat.com>
John
I you have a reason to keep it as it, then convince me ;-) especially
since by the end there is no extra change in either place.
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
