Re: [libvirt] [PATCH v3 14/28] lock_daemon_dispatch: Check for ownerPid rather than ownerId
On 08/30/2018 11:47 PM, John Ferlan wrote:
On 08/27/2018 04:08 AM, Michal Privoznik wrote:
> At the beginning of each dispatch function we check if owner
> attributes were registered (these consist of ID, UUID, PID and
> name). The check then consists of checking if ID is not zero.
> This is not going to work with
> VIR_LOCK_MANAGER_OBJECT_TYPE_DAEMON which doesn't set ID. Switch
> to setting PID which is available for both cases.
>
> Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
> ---
> src/locking/lock_daemon_dispatch.c | 14 +++++++-------
> 1 file changed, 7 insertions(+), 7 deletions(-)
>
BTW: My idea of setting id == -1 for deamon still works without any
change required in/for this patch.
So what would be concerning about using ownerPid would be that over the
lifetime of the host the @pid can recycle; whereas, during the lifetime
of the daemon don't we guarantee that the @id will be ever increasing?
But right now I'm too lazy to go look and see if getting the next id is
through libvirtd or virtlockd.
Not against this, but I need to get feedback from earlier patches and of
course your thoughts on the @id vs. @pid rotation. Plus I need see how
this plays out in future patches.
The fact that @id always increases plays no role here and also is not
true. It's only qemu driver that has increasing IDs. For instance, libxl
driver which also uses virtlockd generates 'random' domain IDs.
Pid rotation is no problem here. What my patches currently implement is:
libvirtd registers itself as owner of the lock (with its PID). It has to
keep the connection to virtlockd open since the first lock() to the very
last unlock(), otherwise virtlockd sees EOF on the connection, and
starts releasing locks associated with the PID (=libvirtd) killing the
PID too [1]. So there is no way another process can emerge with the same
PID and magically keep the locks or something. At the moment libvirtd
dies the connection is closed and virlockd releases the locks. There's
no window for PID wrap.
And since for OBJECT_TYPE_DAEMON there is no 'id' (becuase it doesn't
make sense to have one) we have to meet at common ground => PID. Every
process has one (except kernel jobs, but we do not care about those).
We might as well check for owner name.
Michal
1 - this is correct thing to do. We want virlockd to release all the
resources owned by PID when it sees EOF because it acts like regular
file locks. When a process is dying all file locks it has are also
released. More on that in later patches.