Re: [libvirt] [PATCH 02/20] remote: Don't hard code the feature VIR_DRV_FEATURE_REMOTE_CLOSE_CALLBACK as available

On Thu, Mar 15, 2018 at 12:02 AM +0100, John Ferlan <jferlan(a)redhat.com&gt; wrote: > On 03/14/2018 05:30 PM, John Ferlan wrote: >> >> >> On 03/08/2018 07:20 AM, Marc Hartmayer wrote: >>> Don't assume that the feature VIR_DRV_FEATURE_REMOTE_CLOSE_CALLBACK is >>> available for every driver used for the connection. >>> >>> Signed-off-by: Marc Hartmayer <mhartmay(a)linux.vnet.ibm.com&gt; >>> Reviewed-by: Bjoern Walk <bwalk(a)linux.vnet.ibm.com&gt; >>> Reviewed-by: Boris Fiuczynski <fiuczy(a)linux.vnet.ibm.com&gt; >>> --- >>> src/remote/remote_daemon_dispatch.c | 2 +- >>> 1 file changed, 1 insertion(+), 1 deletion(-) >>> >> >> Something that is not clear about this one - since this was added for >> 'vz' driver by commit id 'f484310a', then shouldn't >> vzConnectSupportsFeature be updated to indicate support? >> >> If I'm right and you add the feature to the vz routine along with a >> reference to the commit id that forgot to in your commit message, >> then You’re right. >> >> Reviewed-by: John Ferlan <jferlan(a)redhat.com&gt; >> >> If I'm wrong - then help me understand! >> >> John >> > > Once I got to patch 5 I started questioning my (limited) understanding > of what's going on here. > > Still if we move the REMOTE_CLOSE_CALLBACK into the "check with the > connection" to see if it's supported, then how does patch 3/virsh > actually utilize the virConnectRegisterCloseCallback unless the vz > driver is enabled? For the vz driver we’ve to add this feature to the 'vzConnectSupportsFeature' function (and for all other internal drivers, which supports the register/unregister close callback interface). > > Won't the check with the connection driver for everyone else return 0? So lets start from the beginning… 'doRemoteOpen' checks if the server has the feature to register a close callback for the connection between the “remote daemon driver” and the used internal driver (e.g. QEMU, bhyve, etc.) (this is cached in priv->serverCloseCallback (bool) on the client side). In my opinion this depends (also) on the internal driver and therefore there is the need to check this by use of 'virConnectSupportsFeature'.

The cover letter '[libvirt] [PATCH v5 0/10] add close callback for drivers with persistent connection' says to the original change: “Currently close callback API can only inform us of closing the connection between remote driver and daemon. But what if a driver running in the daemon itself can have another persistent connection? In this case we want to be informed of that connection changes state too.” 'doRemoteOpen' does the following RPC call in remote_driver.c for the check: remoteConnectSupportsFeatureUnlocked(conn, priv, VIR_DRV_FEATURE_REMOTE_CLOSE_CALLBACK); Before this patch, 'remoteDispatchConnectSupportsFeature' always returned that the feature is supported (regardless of the capability of the used internal driver) => priv->serverCloseCallback is always true on the client side. If now 'remoteConnectRegisterCloseCallback' is called on the client side (virsh calls it in virshReconnect) the client tests for 'priv->serverCloseCallback' and as this is always true, it will call the RPC 'REMOTE_PROC_CONNECT_REGISTER_CLOSE_CALLBACK' => 'remoteDispatchConnectRegisterCloseCallback' is called on the server side. So lets have a look at 'remoteDispatchConnectRegisterCloseCallback'. It registers internally that a close callback has been registered ('priv->closeRegistered = true' (daemon/remote.c)) and calls: if (virConnectRegisterCloseCallback(priv->conn, remoteRelayConnectionClosedEvent, client, NULL) < 0) priv->conn is the connection to the internal driver (e.g. QEMU, vz, etc.). virConnectRegisterCloseCallback is a _public_ API and it returns _only_ an error if the used internal driver implements the 'connectRegisterCloseCallback' function and an error happens during the 'connectRegisterCloseCallback(…)' call (src/libvirt-host.c). Important: virConnect(Un)RegisterCloseCallback throws _no_ unsupported error even if the internal driver doesn’t support this API (this is changed in patch 3 of this series). This works as long as you don't rely on the return value of virConnect(Un)RegisterCloseCallback. I stumbled across the problem as I tried to use 'domainClientEventCallbacks' for 'remoteReplayConnectionClosedEvent' (patch 18 of this series) - actually I produced a memory leak as the callback object was never freed as it was never registered (and I thought it is). So at least that’s my understanding of the code. But for safeness, I added Nikolay to CC. […snip] -- Beste Grüße / Kind regards Marc Hartmayer IBM Deutschland Research & Development GmbH Vorsitzende des Aufsichtsrats: Martina Koederitz Geschäftsführung: Dirk Wittkopp Sitz der Gesellschaft: Böblingen Registergericht: Amtsgericht Stuttgart, HRB 243294