On 09/03/2018 11:13 AM, Michal Privoznik wrote: > 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@xxxxxxxxxx> >>> --- >>> 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. I saw your query on #virt ;-) > > 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. Well, we know our consumer libvirtd is well behaved. Of course recalling offhand while reviewing whether the magic of properly handling what to do on EOF or closure is a challenge. Still based on the fact that @id are random w/ libxl means that while unlikely it is possible for reuse too and at least with PID's we have assurances that the lock manager is handling the "removal" of locks when the EOF is seen, so going with PID's is fine. Maybe a blurb that indicates what lock manager magic allows us to consider why usage of PID's even with the knowledge they can turn over makes it more desirable to use. John > > 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. > -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list
