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

[Nikolay Shirokovskiy]

On 15.03.2018 21:52, Marc Hartmayer wrote:
> On Thu, Mar 15, 2018 at 12:02 AM +0100, John Ferlan <jferlan at redhat.com> 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 at linux.vnet.ibm.com>
>>>> Reviewed-by: Bjoern Walk <bwalk at linux.vnet.ibm.com>
>>>> Reviewed-by: Boris Fiuczynski <fiuczy at linux.vnet.ibm.com>
>>>> ---
>>>>  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 at redhat.com>
>>>
>>> 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'.

Hi, Marc.

I agree with the suggested change. There is no need to register/unregister
close callback in remote driver if internal driver does not implement appropriate feature
(now this is only vz driver). Current code works because if internal driver
does not implement connect(Un)RegisterCloseCallback then virConnectRegisterCloseCallback 
returns success as you mentioned below.

As to changing virConnectRegisterCloseCallback to return "not supported"
- I don't think this possible as this breaks backward compat.

Nikolay

> 
> 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
>