On 08/27/2018 04:08 AM, Michal Privoznik wrote:
> This flag causes connection to be opened when needed (e.g. when
> calling virLockManagerLockDaemonAcquire for the first time) and
> instead of closing it at the end of such API store it in
> privateData so that it can be reused by later calls.
>
> This is needed because if a resource is acquired and connection
> is closed then virtlockd kills the registered PID (that's what
> virtlockd is designed to do). Therefore we will need the
> connection to open at drvAcquire and close not any sooner than
> drvRelease. However, as we will be locking files step-by-step we
> want to avoid opening new connection for every drvAcquire +
> drvRelease pair, so the connection is going to be shared even
> more than that. But more on that in next commit.
>
> Signed-off-by: Michal Privoznik <mprivozn@xxxxxxxxxx>
> ---
> src/locking/lock_driver.h | 7 +++++
> src/locking/lock_driver_lockd.c | 68 +++++++++++++++++++++++++++++++++++++----
> 2 files changed, 69 insertions(+), 6 deletions(-)
>
> diff --git a/src/locking/lock_driver.h b/src/locking/lock_driver.h
> index 59c4c3aac7..7e3ffc58b5 100644
> --- a/src/locking/lock_driver.h
> +++ b/src/locking/lock_driver.h
> @@ -67,8 +67,15 @@ 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),
> + /* Causes driver to keep connection open and reuse it for further use. */
> + VIR_LOCK_MANAGER_ACQUIRE_KEEP_OPEN = (1 << 2),
> } virLockManagerAcquireFlags;
>
> +typedef enum {
> + /* Reuse previously saved connection. */
> + VIR_LOCK_MANAGER_RELEASE_KEEP_OPEN = (1 << 0),
> +} virLockManagerReleaseFlags;
> +
> typedef enum {
> /* virLockManagerNew called for a freshly started domain */
> VIR_LOCK_MANAGER_NEW_STARTED = (1 << 0),
> diff --git a/src/locking/lock_driver_lockd.c b/src/locking/lock_driver_lockd.c
> index 4883e89ac6..14f9eae760 100644
> --- a/src/locking/lock_driver_lockd.c
> +++ b/src/locking/lock_driver_lockd.c
> @@ -76,6 +76,11 @@ struct _virLockManagerLockDaemonPrivate {
>
> size_t nresources;
> virLockManagerLockDaemonResourcePtr resources;
> +
> + int clientRefs;
> + virNetClientPtr client;
> + virNetClientProgramPtr program;
> + int counter;
> };
>
>
> @@ -440,6 +445,13 @@ virLockManagerLockDaemonPrivateFree(virLockManagerLockDaemonPrivatePtr priv)
> default:
> break;
> }
> +
> + if (priv->client) {
> + virNetClientClose(priv->client);
> + virObjectUnref(priv->client);
> + virObjectUnref(priv->program);
> + }
> +
How about a helper method that would do these?
I wonder now if the @priv should keep track of flags as well?
That way you could "always" use @priv to store the client, program, and
counter and then use helper methods to determine at close whether @flags
had the KEEP_OPEN or not instead of having KEEP_OPEN in various places.
> VIR_FREE(priv);
> }
>
> @@ -770,7 +782,8 @@ static int virLockManagerLockDaemonAcquire(virLockManagerPtr lock,
> 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_KEEP_OPEN, -1);
>
> if (priv->type == VIR_LOCK_MANAGER_OBJECT_TYPE_DOMAIN &&
> priv->nresources == 0 &&
> @@ -781,7 +794,14 @@ static int virLockManagerLockDaemonAcquire(virLockManagerPtr lock,
> return -1;
> }
>
> - if (!(client = virLockManagerLockDaemonConnect(lock, &program, &counter)))
> + if (flags & VIR_LOCK_MANAGER_ACQUIRE_KEEP_OPEN) {
> + client = priv->client;
> + program = priv->program;
> + counter = priv->counter;
> + }
> +
> + if (!client &&
> + !(client = virLockManagerLockDaemonConnect(lock, &program, &counter)))
If "always" storing in @priv this alters to if !priv->client &&
!(priv->client ==...
> goto cleanup;
>
> if (fd &&
> @@ -814,11 +834,25 @@ static int virLockManagerLockDaemonAcquire(virLockManagerPtr lock,
> virLockManagerLockDaemonConnectionRestrict(lock, client, program, &counter) < 0)
> goto cleanup;
>
> + if (flags & VIR_LOCK_MANAGER_ACQUIRE_KEEP_OPEN) {
> + VIR_STEAL_PTR(priv->client, client);
> + VIR_STEAL_PTR(priv->program, program);
> + priv->counter = counter;
> + }
> +
If "always" using @priv means this isn't necessary as long as @flags is
also stored.
> rv = 0;
>
> cleanup:
> - if (rv != 0 && fd)
> - VIR_FORCE_CLOSE(*fd);
> + if (rv < 0) {
> + if (fd)
> + VIR_FORCE_CLOSE(*fd);
> +
> + priv->client = NULL;
> + priv->program = NULL;
> + priv->counter = 0;
> + priv->clientRefs = 0;
So one failure this time causes previously stored successes to be thrown
away? Or am I reading too much into this? Why is clientRefs not
auto-incremented here, but auto-decremented in Release?
This is where I'd think a helper would be able to know the "last"
referenced was removed and thus be able to perform the close and free of
resources.
Calling *PrivateFree and finding that there's extra clientRefs
would/could mean something is programatically wrong, right?
> + }
> +
> virNetClientClose(client);
> virObjectUnref(client);
> virObjectUnref(program);
Always storing in @priv, priv->flags, and using @rv I would think could
easily be managed in a helper...
> @@ -837,12 +871,20 @@ static int virLockManagerLockDaemonRelease(virLockManagerPtr lock,
> size_t i;
> virLockManagerLockDaemonPrivatePtr priv = lock->privateData;
>
> - virCheckFlags(0, -1);
> + virCheckFlags(VIR_LOCK_MANAGER_RELEASE_KEEP_OPEN, -1);
>
> if (state)
> *state = NULL;
>
> - if (!(client = virLockManagerLockDaemonConnect(lock, &program, &counter)))
> + if (flags & VIR_LOCK_MANAGER_RELEASE_KEEP_OPEN) {
> + client = priv->client;
> + program = priv->program;
> + counter = priv->counter;
> + priv->clientRefs--;
This decrements something from 0...
> + }
> +
> + if (!client &&
> + !(client = virLockManagerLockDaemonConnect(lock, &program, &counter)))
> goto cleanup;
>
> for (i = 0; i < priv->nresources; i++) {
> @@ -870,9 +912,23 @@ static int virLockManagerLockDaemonRelease(virLockManagerPtr lock,
> goto cleanup;
> }
>
> + if (flags & VIR_LOCK_MANAGER_RELEASE_KEEP_OPEN) {
> + /* Avoid freeing in cleanup. */
> + client = NULL;
> + program = NULL;
> + counter = 0;
> + }
> +
> rv = 0;
>
> cleanup:
> + if (rv < 0) {
> + priv->client = NULL;
> + priv->program = NULL;
> + priv->counter = 0;
> + priv->clientRefs = 0;
> + }
> +
Again, I'm not clear why when this release fails we go off and clear out
everything? Including things that may have been connected before?
Certain a properly documented helper would solve this mystery for me.
One that could manage things based on KEEP_OPEN, @priv, and @rv. I would
assume the close open when KEEP_OPEN is true would be where the
auto-decrement would occur and be checked to determine whether the
subsequent closes and unref's happen.
John
> virNetClientClose(client);
> virObjectUnref(client);
> virObjectUnref(program);
>
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
