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

[Marc Hartmayer]

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

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