Re: [libvirt] [PATCH v2 10/12] qemu_hotplug: Hotplug of reservations

On 02/21/2018 01:11 PM, Michal Privoznik wrote: But wasn't there a very special reason to start it between fork and exec? Does that still hold true? That is why can we start the daemon after exec now, but we couldn't before in the previous patch? It feels quite "disjoint" to have the unplug in a subsequent patch too. Considering them both in one just seems "better", but it's not a deal breaker. qemuBuild? Why not qemuDomainAdd? ohh, umm, in qemuDomainAttachDiskGeneric there's a : srcPriv = QEMU_DOMAIN_STORAGE_SOURCE_PRIVATE(disk->src); if (srcPriv) { secinfo = srcPriv->secinfo; encinfo = srcPriv->encinfo; } Which makes light dawn that it "was" possible for srcPriv == NULL and the "thought" that the subsequent deref is going to fail rather spectacularly. See commit '8056721cb' (and a couple of others). Although it seems patch 4 and your change to qemuDomainPrepareDiskSource to call qemuDomainStorageSourcePrivateNew instead of having it called in qemuDomainSecretStorageSourcePrepare would seem to ensure that disk source privateData is always allocated now - meaning a number of other places where srcPriv is/was checked are no longer necessary. Maybe that one change needs to be "extracted" out to make things obvious. Not required, but just thinking and typing thoughts. qemuDestroy? Why not qemuDomainDel? BTW: This is where I'd expect to remove the object and then... Just unlink the path... See some thoughts below... So for one of the managed modes (as noted much earlier) we could be creating a object with a duplicated alias - does that work? I thought id= has to be unique. oh, I see, you're mixing the way TLS object tear down occurred and how other objects happen. If you're going to mimic TLS, then change qemuBuildPRDefInfoProps to be something like qemuDomainAddPRDefObject which would do the EnterMonitorAsync, Props mgmt, AddObject, and ExitMonitor. That way qemuDestroyPRDefObject changes to qemuDomainDelPRDefObject which would handle the failure path. Then I believe you don't have to pass around prAlias and prPath since they'd be "obtainable". This subsequent hunk feels like it could have gone in for the previous patch. Or at the very least some function that would unlink the socket path for each of the sockets in ndisks that were created. Then that single unlink API gets exposed. The only time force == true is when socketPath == NULL... and that's only in the shutdown path. When socketPath != NULL, force == false, and we're going to unplug the socket, right? Perhaps this would be cleaner if only @socketPath was in play. If NULL, then walk the ndisks looking for paths that you'll unlink first before killing the daemon. I actually think having a separate unlink the socket would be just fine. Does it really matter if it's the "last" one to stop the helper daemon? All I can picture is some test that continually adds and removes one socket and that just thrashing (or eventually failing) because of the startup processing. IMO: Once you start the daemon because there was a lun wanting reservations - just keep it running even if the last possible consumer was unplugged from the guest. But yes, I do understand why you would stop it if it was the last one, I'm just thinking it may not be necessary. I could be off in the weeds - as this PR processing still isn't as clear to me as it should be by this point. John