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@linux.vnet.ibm.com>
Reviewed-by: Bjoern Walk <bwalk@linux.vnet.ibm.com>
Reviewed-by: Boris Fiuczynski <fiuczy@linux.vnet.ibm.com>
---
src/remote/remote_daemon_dispatch.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/remote/remote_daemon_dispatch.c b/src/remote/remote_daemon_dispatch.c
index 82f6400ca49d..bf6c00348a5e 100644
--- a/src/remote/remote_daemon_dispatch.c
+++ b/src/remote/remote_daemon_dispatch.c
@@ -4667,7 +4667,6 @@ static int remoteDispatchConnectSupportsFeature(virNetServerPtr server ATTRIBUTE
switch ((virDrvFeature) args->feature) {
case VIR_DRV_FEATURE_FD_PASSING:
case VIR_DRV_FEATURE_REMOTE_EVENT_CALLBACK:
- case VIR_DRV_FEATURE_REMOTE_CLOSE_CALLBACK:
supported = 1;
break;
case VIR_DRV_FEATURE_MIGRATION_V1:
@@ -4681,6 +4680,7 @@ static int remoteDispatchConnectSupportsFeature(virNetServerPtr server ATTRIBUTE
case VIR_DRV_FEATURE_XML_MIGRATABLE:
case VIR_DRV_FEATURE_MIGRATION_OFFLINE:
case VIR_DRV_FEATURE_MIGRATION_PARAMS:
+ case VIR_DRV_FEATURE_REMOTE_CLOSE_CALLBACK:
default:
if ((supported = virConnectSupportsFeature(priv->conn, args->feature)) < 0)
goto cleanup;
--
2.13.4
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
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@linux.vnet.ibm.com>
> Reviewed-by: Bjoern Walk <bwalk@linux.vnet.ibm.com>
> Reviewed-by: Boris Fiuczynski <fiuczy@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
Reviewed-by: John Ferlan <jferlan@redhat.com>
If I'm wrong - then help me understand!
John
> diff --git a/src/remote/remote_daemon_dispatch.c b/src/remote/remote_daemon_dispatch.c
> index 82f6400ca49d..bf6c00348a5e 100644
> --- a/src/remote/remote_daemon_dispatch.c
> +++ b/src/remote/remote_daemon_dispatch.c
> @@ -4667,7 +4667,6 @@ static int remoteDispatchConnectSupportsFeature(virNetServerPtr server ATTRIBUTE
> switch ((virDrvFeature) args->feature) {
> case VIR_DRV_FEATURE_FD_PASSING:
> case VIR_DRV_FEATURE_REMOTE_EVENT_CALLBACK:
> - case VIR_DRV_FEATURE_REMOTE_CLOSE_CALLBACK:
> supported = 1;
> break;
> case VIR_DRV_FEATURE_MIGRATION_V1:
> @@ -4681,6 +4680,7 @@ static int remoteDispatchConnectSupportsFeature(virNetServerPtr server ATTRIBUTE
> case VIR_DRV_FEATURE_XML_MIGRATABLE:
> case VIR_DRV_FEATURE_MIGRATION_OFFLINE:
> case VIR_DRV_FEATURE_MIGRATION_PARAMS:
> + case VIR_DRV_FEATURE_REMOTE_CLOSE_CALLBACK:
> default:
> if ((supported = virConnectSupportsFeature(priv->conn, args->feature)) < 0)
> goto cleanup;
>
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
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@linux.vnet.ibm.com>
>> Reviewed-by: Bjoern Walk <bwalk@linux.vnet.ibm.com>
>> Reviewed-by: Boris Fiuczynski <fiuczy@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
>
> Reviewed-by: John Ferlan <jferlan@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?
Won't the check with the connection driver for everyone else return 0?
Maybe I just need to understand this code a bit more <sigh>
John
>> diff --git a/src/remote/remote_daemon_dispatch.c b/src/remote/remote_daemon_dispatch.c
>> index 82f6400ca49d..bf6c00348a5e 100644
>> --- a/src/remote/remote_daemon_dispatch.c
>> +++ b/src/remote/remote_daemon_dispatch.c
>> @@ -4667,7 +4667,6 @@ static int remoteDispatchConnectSupportsFeature(virNetServerPtr server ATTRIBUTE
>> switch ((virDrvFeature) args->feature) {
>> case VIR_DRV_FEATURE_FD_PASSING:
>> case VIR_DRV_FEATURE_REMOTE_EVENT_CALLBACK:
>> - case VIR_DRV_FEATURE_REMOTE_CLOSE_CALLBACK:
>> supported = 1;
>> break;
>> case VIR_DRV_FEATURE_MIGRATION_V1:
>> @@ -4681,6 +4680,7 @@ static int remoteDispatchConnectSupportsFeature(virNetServerPtr server ATTRIBUTE
>> case VIR_DRV_FEATURE_XML_MIGRATABLE:
>> case VIR_DRV_FEATURE_MIGRATION_OFFLINE:
>> case VIR_DRV_FEATURE_MIGRATION_PARAMS:
>> + case VIR_DRV_FEATURE_REMOTE_CLOSE_CALLBACK:
>> default:
>> if ((supported = virConnectSupportsFeature(priv->conn, args->feature)) < 0)
>> goto cleanup;
>>
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On Thu, Mar 15, 2018 at 12:02 AM +0100, John Ferlan <jferlan@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@linux.vnet.ibm.com>
>>> Reviewed-by: Bjoern Walk <bwalk@linux.vnet.ibm.com>
>>> Reviewed-by: Boris Fiuczynski <fiuczy@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@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
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On 15.03.2018 21:52, Marc Hartmayer wrote:
> On Thu, Mar 15, 2018 at 12:02 AM +0100, John Ferlan <jferlan@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@linux.vnet.ibm.com>
>>>> Reviewed-by: Bjoern Walk <bwalk@linux.vnet.ibm.com>
>>>> Reviewed-by: Boris Fiuczynski <fiuczy@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@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
>
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On Thu, Mar 08, 2018 at 01:20:25PM +0100, Marc Hartmayer wrote: > Don't assume that the feature VIR_DRV_FEATURE_REMOTE_CLOSE_CALLBACK is > available for every driver used for the connection. The very definition of the virConnectRegisterCloseCallback() API is that it will always succeed. What varies is that some driver connections may never fail and so the close callback will never be invoked. The actual registration of the callback will always succeed regardless of which driver is in use. So it is correct to report the VIR_DRV_FEATURE_REMOTE_CLOSE_CALLBACK as always supported regardless of driver. Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :| -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On Mon, Mar 19, 2018 at 04:06 PM +0100, "Daniel P. Berrangé" <berrange@redhat.com> wrote: > On Thu, Mar 08, 2018 at 01:20:25PM +0100, Marc Hartmayer wrote: >> Don't assume that the feature VIR_DRV_FEATURE_REMOTE_CLOSE_CALLBACK is >> available for every driver used for the connection. > > The very definition of the virConnectRegisterCloseCallback() API is that > it will always succeed. What varies is that some driver connections > may never fail and so the close callback will never be invoked. The actual > registration of the callback will always succeed regardless of which driver > is in use. So it is correct to report the VIR_DRV_FEATURE_REMOTE_CLOSE_CALLBACK > as always supported regardless of driver. Okay. So how can we deal with the situation in patch 18 if we cannot differentiate between 'callback was “really” registered' and only the call was 'successful'? If it’s not really registered nobody will free the callback data. This was also the cause for the fix: ce35122cfe: "daemon: fixup refcounting in close callback handling". Thanks in advance. > > > Regards, > Daniel > -- > |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| > |: https://libvirt.org -o- https://fstop138.berrange.com :| > |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :| > -- 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 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On Wed, Mar 21, 2018 at 07:57 AM +0100, Marc Hartmayer <mhartmay@linux.vnet.ibm.com> wrote: > On Mon, Mar 19, 2018 at 04:06 PM +0100, "Daniel P. Berrangé" <berrange@redhat.com> wrote: >> On Thu, Mar 08, 2018 at 01:20:25PM +0100, Marc Hartmayer wrote: >>> Don't assume that the feature VIR_DRV_FEATURE_REMOTE_CLOSE_CALLBACK is >>> available for every driver used for the connection. >> >> The very definition of the virConnectRegisterCloseCallback() API is that >> it will always succeed. What varies is that some driver connections >> may never fail and so the close callback will never be invoked. The actual >> registration of the callback will always succeed regardless of which driver >> is in use. So it is correct to report the VIR_DRV_FEATURE_REMOTE_CLOSE_CALLBACK >> as always supported regardless of driver. > > Okay. So how can we deal with the situation in patch 18 if we cannot > differentiate between 'callback was “really” registered' and only the > call was 'successful'? If it’s not really registered nobody will free > the callback data. This was also the cause for the fix: ce35122cfe: > "daemon: fixup refcounting in close callback handling". > > Thanks in advance. Polite ping. > >> >> >> Regards, >> Daniel >> -- >> |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| >> |: https://libvirt.org -o- https://fstop138.berrange.com :| >> |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :| >> > -- > 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 -- 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 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On Tue, Apr 03, 2018 at 01:46:13PM +0200, Marc Hartmayer wrote: > On Wed, Mar 21, 2018 at 07:57 AM +0100, Marc Hartmayer <mhartmay@linux.vnet.ibm.com> wrote: > > On Mon, Mar 19, 2018 at 04:06 PM +0100, "Daniel P. Berrangé" <berrange@redhat.com> wrote: > >> On Thu, Mar 08, 2018 at 01:20:25PM +0100, Marc Hartmayer wrote: > >>> Don't assume that the feature VIR_DRV_FEATURE_REMOTE_CLOSE_CALLBACK is > >>> available for every driver used for the connection. > >> > >> The very definition of the virConnectRegisterCloseCallback() API is that > >> it will always succeed. What varies is that some driver connections > >> may never fail and so the close callback will never be invoked. The actual > >> registration of the callback will always succeed regardless of which driver > >> is in use. So it is correct to report the VIR_DRV_FEATURE_REMOTE_CLOSE_CALLBACK > >> as always supported regardless of driver. > > > > Okay. So how can we deal with the situation in patch 18 if we cannot > > differentiate between 'callback was “really” registered' and only the > > call was 'successful'? If it’s not really registered nobody will free > > the callback data. This was also the cause for the fix: ce35122cfe: > > "daemon: fixup refcounting in close callback handling". > > > > Thanks in advance. > > Polite ping. If conn->driver->connectRegisterCloseCallback is NULL, then the virConnectRegisterCloseCallback() method should immediately call freecb(opaque). Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :| -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On Tue, Apr 03, 2018 at 02:05 PM +0200, "Daniel P. Berrangé" <berrange@redhat.com> wrote: > On Tue, Apr 03, 2018 at 01:46:13PM +0200, Marc Hartmayer wrote: >> On Wed, Mar 21, 2018 at 07:57 AM +0100, Marc Hartmayer <mhartmay@linux.vnet.ibm.com> wrote: >> > On Mon, Mar 19, 2018 at 04:06 PM +0100, "Daniel P. Berrangé" <berrange@redhat.com> wrote: >> >> On Thu, Mar 08, 2018 at 01:20:25PM +0100, Marc Hartmayer wrote: >> >>> Don't assume that the feature VIR_DRV_FEATURE_REMOTE_CLOSE_CALLBACK is >> >>> available for every driver used for the connection. >> >> >> >> The very definition of the virConnectRegisterCloseCallback() API is that >> >> it will always succeed. What varies is that some driver connections >> >> may never fail and so the close callback will never be invoked. The actual >> >> registration of the callback will always succeed regardless of which driver >> >> is in use. So it is correct to report the VIR_DRV_FEATURE_REMOTE_CLOSE_CALLBACK >> >> as always supported regardless of driver. >> > >> > Okay. So how can we deal with the situation in patch 18 if we cannot >> > differentiate between 'callback was “really” registered' and only the >> > call was 'successful'? If it’s not really registered nobody will free >> > the callback data. This was also the cause for the fix: ce35122cfe: >> > "daemon: fixup refcounting in close callback handling". >> > >> > Thanks in advance. >> >> Polite ping. > > If conn->driver->connectRegisterCloseCallback is NULL, then the > virConnectRegisterCloseCallback() method should immediately > call freecb(opaque). Thanks! I’ll send a v2. > > > Regards, > Daniel > -- > |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| > |: https://libvirt.org -o- https://fstop138.berrange.com :| > |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :| > -- 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 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
© 2016 - 2025 Red Hat, Inc.