This allows us to get rid of another usage of the global variable
remoteProgram.
Signed-off-by: Marc Hartmayer <mhartmay@linux.vnet.ibm.com>
Reviewed-by: Boris Fiuczynski <fiuczy@linux.vnet.ibm.com>
---
src/remote/remote_daemon_dispatch.c | 18 ++++++++++++++----
1 file changed, 14 insertions(+), 4 deletions(-)
diff --git a/src/remote/remote_daemon_dispatch.c b/src/remote/remote_daemon_dispatch.c
index b4c0e01b0832..cf2cd0add7d6 100644
--- a/src/remote/remote_daemon_dispatch.c
+++ b/src/remote/remote_daemon_dispatch.c
@@ -1661,12 +1661,12 @@ remoteRelayDomainQemuMonitorEvent(virConnectPtr conn,
static
void remoteRelayConnectionClosedEvent(virConnectPtr conn ATTRIBUTE_UNUSED, int reason, void *opaque)
{
- virNetServerClientPtr client = opaque;
+ daemonClientEventCallbackPtr callback = opaque;
VIR_DEBUG("Relaying connection closed event, reason %d", reason);
remote_connect_event_connection_closed_msg msg = { reason };
- remoteDispatchObjectEventSend(client, remoteProgram,
+ remoteDispatchObjectEventSend(callback->client, callback->program,
REMOTE_PROC_CONNECT_EVENT_CONNECTION_CLOSED,
(xdrproc_t)xdr_remote_connect_event_connection_closed_msg,
&msg);
@@ -3836,6 +3836,7 @@ remoteDispatchConnectRegisterCloseCallback(virNetServerPtr server ATTRIBUTE_UNUS
virNetMessageErrorPtr rerr)
{
int rv = -1;
+ daemonClientEventCallbackPtr callback = NULL;
struct daemonClientPrivate *priv =
virNetServerClientGetPrivateData(client);
@@ -3846,9 +3847,16 @@ remoteDispatchConnectRegisterCloseCallback(virNetServerPtr server ATTRIBUTE_UNUS
goto cleanup;
}
+ if (VIR_ALLOC(callback) < 0)
+ goto cleanup;
+ callback->client = virObjectRef(client);
+ callback->program = virObjectRef(remoteProgram);
+ /* eventID, callbackID, and legacy are not used */
+ callback->eventID = -1;
+ callback->callbackID = -1;
if (virConnectRegisterCloseCallback(priv->conn,
remoteRelayConnectionClosedEvent,
- client, NULL) < 0)
+ callback, remoteEventCallbackFree) < 0)
goto cleanup;
priv->closeRegistered = true;
@@ -3856,8 +3864,10 @@ remoteDispatchConnectRegisterCloseCallback(virNetServerPtr server ATTRIBUTE_UNUS
cleanup:
virMutexUnlock(&priv->lock);
- if (rv < 0)
+ if (rv < 0) {
+ remoteEventCallbackFree(callback);
virNetMessageSaveError(rerr);
+ }
return rv;
}
--
2.13.4
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On 04/12/2018 08:40 AM, Marc Hartmayer wrote:
> This allows us to get rid of another usage of the global variable
> remoteProgram.
>
> Signed-off-by: Marc Hartmayer <mhartmay@linux.vnet.ibm.com>
> Reviewed-by: Boris Fiuczynski <fiuczy@linux.vnet.ibm.com>
> ---
> src/remote/remote_daemon_dispatch.c | 18 ++++++++++++++----
> 1 file changed, 14 insertions(+), 4 deletions(-)
>
> diff --git a/src/remote/remote_daemon_dispatch.c b/src/remote/remote_daemon_dispatch.c
> index b4c0e01b0832..cf2cd0add7d6 100644
> --- a/src/remote/remote_daemon_dispatch.c
> +++ b/src/remote/remote_daemon_dispatch.c
> @@ -1661,12 +1661,12 @@ remoteRelayDomainQemuMonitorEvent(virConnectPtr conn,
> static
> void remoteRelayConnectionClosedEvent(virConnectPtr conn ATTRIBUTE_UNUSED, int reason, void *opaque)
> {
> - virNetServerClientPtr client = opaque;
> + daemonClientEventCallbackPtr callback = opaque;
>
> VIR_DEBUG("Relaying connection closed event, reason %d", reason);
>
> remote_connect_event_connection_closed_msg msg = { reason };
> - remoteDispatchObjectEventSend(client, remoteProgram,
> + remoteDispatchObjectEventSend(callback->client, callback->program,
> REMOTE_PROC_CONNECT_EVENT_CONNECTION_CLOSED,
> (xdrproc_t)xdr_remote_connect_event_connection_closed_msg,
> &msg);
> @@ -3836,6 +3836,7 @@ remoteDispatchConnectRegisterCloseCallback(virNetServerPtr server ATTRIBUTE_UNUS
> virNetMessageErrorPtr rerr)
> {
> int rv = -1;
> + daemonClientEventCallbackPtr callback = NULL;
> struct daemonClientPrivate *priv =
> virNetServerClientGetPrivateData(client);
>
> @@ -3846,9 +3847,16 @@ remoteDispatchConnectRegisterCloseCallback(virNetServerPtr server ATTRIBUTE_UNUS
> goto cleanup;
> }
>
> + if (VIR_ALLOC(callback) < 0)
> + goto cleanup;
> + callback->client = virObjectRef(client);
Oh and this would seem to fix a problem with the callback possibly using
@client after it had been freed!
> + callback->program = virObjectRef(remoteProgram);
> + /* eventID, callbackID, and legacy are not used */
> + callback->eventID = -1;
> + callback->callbackID = -1;
> if (virConnectRegisterCloseCallback(priv->conn,
> remoteRelayConnectionClosedEvent,
> - client, NULL) < 0)
> + callback, remoteEventCallbackFree) < 0)
> goto cleanup;
>
@callback would be leaked in the normal path... If you consider all the
other callbacks will load @callback onto some list that gets run during
remoteClientFreePrivateCallbacks via DEREG_CB, this code would need to
do something similar. Since there's only 1 it's perhaps easier at least.
> priv->closeRegistered = true;
Perhaps change closeRegistered from bool to daemonClientEventCallbackPtr
and handle it that way similar to how other callback processing is handled.
John
> @@ -3856,8 +3864,10 @@ remoteDispatchConnectRegisterCloseCallback(virNetServerPtr server ATTRIBUTE_UNUS
>
> cleanup:
> virMutexUnlock(&priv->lock);
> - if (rv < 0)
> + if (rv < 0) {
> + remoteEventCallbackFree(callback);
> virNetMessageSaveError(rerr);
> + }
> return rv;
> }
>
>
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On Thu, Apr 26, 2018 at 05:07 PM +0200, John Ferlan <jferlan@redhat.com> wrote:
> On 04/12/2018 08:40 AM, Marc Hartmayer wrote:
>> This allows us to get rid of another usage of the global variable
>> remoteProgram.
>>
>> Signed-off-by: Marc Hartmayer <mhartmay@linux.vnet.ibm.com>
>> Reviewed-by: Boris Fiuczynski <fiuczy@linux.vnet.ibm.com>
>> ---
>> src/remote/remote_daemon_dispatch.c | 18 ++++++++++++++----
>> 1 file changed, 14 insertions(+), 4 deletions(-)
>>
>> diff --git a/src/remote/remote_daemon_dispatch.c b/src/remote/remote_daemon_dispatch.c
>> index b4c0e01b0832..cf2cd0add7d6 100644
>> --- a/src/remote/remote_daemon_dispatch.c
>> +++ b/src/remote/remote_daemon_dispatch.c
>> @@ -1661,12 +1661,12 @@ remoteRelayDomainQemuMonitorEvent(virConnectPtr conn,
>> static
>> void remoteRelayConnectionClosedEvent(virConnectPtr conn ATTRIBUTE_UNUSED, int reason, void *opaque)
>> {
>> - virNetServerClientPtr client = opaque;
>> + daemonClientEventCallbackPtr callback = opaque;
>>
>> VIR_DEBUG("Relaying connection closed event, reason %d", reason);
>>
>> remote_connect_event_connection_closed_msg msg = { reason };
>> - remoteDispatchObjectEventSend(client, remoteProgram,
>> + remoteDispatchObjectEventSend(callback->client, callback->program,
>> REMOTE_PROC_CONNECT_EVENT_CONNECTION_CLOSED,
>> (xdrproc_t)xdr_remote_connect_event_connection_closed_msg,
>> &msg);
>> @@ -3836,6 +3836,7 @@ remoteDispatchConnectRegisterCloseCallback(virNetServerPtr server ATTRIBUTE_UNUS
>> virNetMessageErrorPtr rerr)
>> {
>> int rv = -1;
>> + daemonClientEventCallbackPtr callback = NULL;
>> struct daemonClientPrivate *priv =
>> virNetServerClientGetPrivateData(client);
>>
>> @@ -3846,9 +3847,16 @@ remoteDispatchConnectRegisterCloseCallback(virNetServerPtr server ATTRIBUTE_UNUS
>> goto cleanup;
>> }
>>
>> + if (VIR_ALLOC(callback) < 0)
>> + goto cleanup;
>> + callback->client = virObjectRef(client);
>
> Oh and this would seem to fix a problem with the callback possibly using
> @client after it had been freed!
The problem is more of a theoretical nature as Nikolay had explained:
“Refcounting was here originally but then removed in [1] as it conflicts with
virConnectRegisterCloseCallback returns 0 in case connectRegisterCloseCallback
is not implemented. This is safe though (at least nobody touch this place :).
[1] ce35122cfe: "daemon: fixup refcounting in close callback handling"”
>
>> + callback->program = virObjectRef(remoteProgram);
>> + /* eventID, callbackID, and legacy are not used */
>> + callback->eventID = -1;
>> + callback->callbackID = -1;
>> if (virConnectRegisterCloseCallback(priv->conn,
>> remoteRelayConnectionClosedEvent,
>> - client, NULL) < 0)
>> + callback, remoteEventCallbackFree) < 0)
>> goto cleanup;
>>
>
> @callback would be leaked in the normal path...
By normal path, you mean without the first patch?
> If you consider all the
> other callbacks will load @callback onto some list that gets run during
> remoteClientFreePrivateCallbacks via DEREG_CB, this code would need to
> do something similar. Since there's only 1 it's perhaps easier at
> least.
>
>> priv->closeRegistered = true;
>
> Perhaps change closeRegistered from bool to daemonClientEventCallbackPtr
> and handle it that way similar to how other callback processing is
> handled.
This would be one way how to deal with it (even without the first
patch). But a double free error must be prevented.
>
> John
>
>> @@ -3856,8 +3864,10 @@ remoteDispatchConnectRegisterCloseCallback(virNetServerPtr server ATTRIBUTE_UNUS
>>
>> cleanup:
>> virMutexUnlock(&priv->lock);
>> - if (rv < 0)
>> + if (rv < 0) {
>> + remoteEventCallbackFree(callback);
>> virNetMessageSaveError(rerr);
>> + }
>> return rv;
>> }
>>
>>
>
--
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 04/26/2018 12:27 PM, Marc Hartmayer wrote:
> On Thu, Apr 26, 2018 at 05:07 PM +0200, John Ferlan <jferlan@redhat.com> wrote:
>> On 04/12/2018 08:40 AM, Marc Hartmayer wrote:
>>> This allows us to get rid of another usage of the global variable
>>> remoteProgram.
>>>
>>> Signed-off-by: Marc Hartmayer <mhartmay@linux.vnet.ibm.com>
>>> Reviewed-by: Boris Fiuczynski <fiuczy@linux.vnet.ibm.com>
>>> ---
>>> src/remote/remote_daemon_dispatch.c | 18 ++++++++++++++----
>>> 1 file changed, 14 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/src/remote/remote_daemon_dispatch.c b/src/remote/remote_daemon_dispatch.c
>>> index b4c0e01b0832..cf2cd0add7d6 100644
>>> --- a/src/remote/remote_daemon_dispatch.c
>>> +++ b/src/remote/remote_daemon_dispatch.c
>>> @@ -1661,12 +1661,12 @@ remoteRelayDomainQemuMonitorEvent(virConnectPtr conn,
>>> static
>>> void remoteRelayConnectionClosedEvent(virConnectPtr conn ATTRIBUTE_UNUSED, int reason, void *opaque)
>>> {
>>> - virNetServerClientPtr client = opaque;
>>> + daemonClientEventCallbackPtr callback = opaque;
>>>
>>> VIR_DEBUG("Relaying connection closed event, reason %d", reason);
>>>
>>> remote_connect_event_connection_closed_msg msg = { reason };
>>> - remoteDispatchObjectEventSend(client, remoteProgram,
>>> + remoteDispatchObjectEventSend(callback->client, callback->program,
>>> REMOTE_PROC_CONNECT_EVENT_CONNECTION_CLOSED,
>>> (xdrproc_t)xdr_remote_connect_event_connection_closed_msg,
>>> &msg);
>>> @@ -3836,6 +3836,7 @@ remoteDispatchConnectRegisterCloseCallback(virNetServerPtr server ATTRIBUTE_UNUS
>>> virNetMessageErrorPtr rerr)
>>> {
>>> int rv = -1;
>>> + daemonClientEventCallbackPtr callback = NULL;
>>> struct daemonClientPrivate *priv =
>>> virNetServerClientGetPrivateData(client);
>>>
>>> @@ -3846,9 +3847,16 @@ remoteDispatchConnectRegisterCloseCallback(virNetServerPtr server ATTRIBUTE_UNUS
>>> goto cleanup;
>>> }
>>>
>>> + if (VIR_ALLOC(callback) < 0)
>>> + goto cleanup;
>>> + callback->client = virObjectRef(client);
>>
>> Oh and this would seem to fix a problem with the callback possibly using
>> @client after it had been freed!
>
> The problem is more of a theoretical nature as Nikolay had explained:
>
> “Refcounting was here originally but then removed in [1] as it conflicts with
> virConnectRegisterCloseCallback returns 0 in case connectRegisterCloseCallback
> is not implemented. This is safe though (at least nobody touch this place :).
>
> [1] ce35122cfe: "daemon: fixup refcounting in close callback handling"”
>
>>
>>> + callback->program = virObjectRef(remoteProgram);
>>> + /* eventID, callbackID, and legacy are not used */
>>> + callback->eventID = -1;
>>> + callback->callbackID = -1;
>>> if (virConnectRegisterCloseCallback(priv->conn,
>>> remoteRelayConnectionClosedEvent,
>>> - client, NULL) < 0)
>>> + callback, remoteEventCallbackFree) < 0)
>>> goto cleanup;
>>>
>>
>> @callback would be leaked in the normal path...
>
> By normal path, you mean without the first patch?
>
I was following how the other register functions proceeded and they
saved the callback in a list to be freed at unregister. So if Register
succeeds, rv == 0 and the removeEventCallbackFree(callback) isn't called
and we leak callback. At least that's how I read it
John
>> If you consider all the
>> other callbacks will load @callback onto some list that gets run during
>> remoteClientFreePrivateCallbacks via DEREG_CB, this code would need to
>> do something similar. Since there's only 1 it's perhaps easier at
>> least.
>>
>>> priv->closeRegistered = true;
>>
>> Perhaps change closeRegistered from bool to daemonClientEventCallbackPtr
>> and handle it that way similar to how other callback processing is
>> handled.
>
> This would be one way how to deal with it (even without the first
> patch). But a double free error must be prevented.
>
>>
>> John
>>
>>> @@ -3856,8 +3864,10 @@ remoteDispatchConnectRegisterCloseCallback(virNetServerPtr server ATTRIBUTE_UNUS
>>>
>>> cleanup:
>>> virMutexUnlock(&priv->lock);
>>> - if (rv < 0)
>>> + if (rv < 0) {
>>> + remoteEventCallbackFree(callback);
>>> virNetMessageSaveError(rerr);
>>> + }
>>> return rv;
>>> }
>>>
>>>
>>
> --
> 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 Fri, Apr 27, 2018 at 02:19 AM +0200, John Ferlan <jferlan@redhat.com> wrote:
> On 04/26/2018 12:27 PM, Marc Hartmayer wrote:
>> On Thu, Apr 26, 2018 at 05:07 PM +0200, John Ferlan <jferlan@redhat.com> wrote:
>>> On 04/12/2018 08:40 AM, Marc Hartmayer wrote:
>>>> This allows us to get rid of another usage of the global variable
>>>> remoteProgram.
>>>>
>>>> Signed-off-by: Marc Hartmayer <mhartmay@linux.vnet.ibm.com>
>>>> Reviewed-by: Boris Fiuczynski <fiuczy@linux.vnet.ibm.com>
>>>> ---
>>>> src/remote/remote_daemon_dispatch.c | 18 ++++++++++++++----
>>>> 1 file changed, 14 insertions(+), 4 deletions(-)
>>>>
>>>> diff --git a/src/remote/remote_daemon_dispatch.c b/src/remote/remote_daemon_dispatch.c
>>>> index b4c0e01b0832..cf2cd0add7d6 100644
>>>> --- a/src/remote/remote_daemon_dispatch.c
>>>> +++ b/src/remote/remote_daemon_dispatch.c
>>>> @@ -1661,12 +1661,12 @@ remoteRelayDomainQemuMonitorEvent(virConnectPtr conn,
>>>> static
>>>> void remoteRelayConnectionClosedEvent(virConnectPtr conn ATTRIBUTE_UNUSED, int reason, void *opaque)
>>>> {
>>>> - virNetServerClientPtr client = opaque;
>>>> + daemonClientEventCallbackPtr callback = opaque;
>>>>
>>>> VIR_DEBUG("Relaying connection closed event, reason %d", reason);
>>>>
>>>> remote_connect_event_connection_closed_msg msg = { reason };
>>>> - remoteDispatchObjectEventSend(client, remoteProgram,
>>>> + remoteDispatchObjectEventSend(callback->client, callback->program,
>>>> REMOTE_PROC_CONNECT_EVENT_CONNECTION_CLOSED,
>>>> (xdrproc_t)xdr_remote_connect_event_connection_closed_msg,
>>>> &msg);
>>>> @@ -3836,6 +3836,7 @@ remoteDispatchConnectRegisterCloseCallback(virNetServerPtr server ATTRIBUTE_UNUS
>>>> virNetMessageErrorPtr rerr)
>>>> {
>>>> int rv = -1;
>>>> + daemonClientEventCallbackPtr callback = NULL;
>>>> struct daemonClientPrivate *priv =
>>>> virNetServerClientGetPrivateData(client);
>>>>
>>>> @@ -3846,9 +3847,16 @@ remoteDispatchConnectRegisterCloseCallback(virNetServerPtr server ATTRIBUTE_UNUS
>>>> goto cleanup;
>>>> }
>>>>
>>>> + if (VIR_ALLOC(callback) < 0)
>>>> + goto cleanup;
>>>> + callback->client = virObjectRef(client);
>>>
>>> Oh and this would seem to fix a problem with the callback possibly using
>>> @client after it had been freed!
>>
>> The problem is more of a theoretical nature as Nikolay had explained:
>>
>> “Refcounting was here originally but then removed in [1] as it conflicts with
>> virConnectRegisterCloseCallback returns 0 in case connectRegisterCloseCallback
>> is not implemented. This is safe though (at least nobody touch this place :).
>>
>> [1] ce35122cfe: "daemon: fixup refcounting in close callback handling"”
>>
>>>
>>>> + callback->program = virObjectRef(remoteProgram);
>>>> + /* eventID, callbackID, and legacy are not used */
>>>> + callback->eventID = -1;
>>>> + callback->callbackID = -1;
>>>> if (virConnectRegisterCloseCallback(priv->conn,
>>>> remoteRelayConnectionClosedEvent,
>>>> - client, NULL) < 0)
>>>> + callback, remoteEventCallbackFree) < 0)
>>>> goto cleanup;
>>>>
>>>
>>> @callback would be leaked in the normal path...
>>
>> By normal path, you mean without the first patch?
>>
>
> I was following how the other register functions proceeded and they
> saved the callback in a list to be freed at unregister. So if Register
> succeeds, rv == 0 and the removeEventCallbackFree(callback) isn't called
> and we leak callback. At least that's how I read it
First - sorry for the late response!
The unregister/free’ing is either done within the
'remoteClientFreePrivateCallbacks' call or by an explicit call of
'remoteDispatchConnectUnregisterCloseCallback'. Do we agree on this? If
yes: the function 'remoteClientFreePrivateCallbacks' calls the
virConnectUnregisterCloseCallback -> remoteEventCallbackFree => no
memory leak.
There will be only a memory leak if the virConnectRegisterCloseCallback
call succeeds but the used driver had no support for registering a close
callback (conn->driver->connectRegisterCloseCallback was NULL) AND the
first patch of this series were not applied.
Did I miss something?
>
[…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
[...] >>>> >>>>> + callback->program = virObjectRef(remoteProgram); >>>>> + /* eventID, callbackID, and legacy are not used */ >>>>> + callback->eventID = -1; >>>>> + callback->callbackID = -1; >>>>> if (virConnectRegisterCloseCallback(priv->conn, >>>>> remoteRelayConnectionClosedEvent, >>>>> - client, NULL) < 0) >>>>> + callback, remoteEventCallbackFree) < 0) >>>>> goto cleanup; >>>>> >>>> >>>> @callback would be leaked in the normal path... >>> >>> By normal path, you mean without the first patch? >>> >> >> I was following how the other register functions proceeded and they >> saved the callback in a list to be freed at unregister. So if Register >> succeeds, rv == 0 and the removeEventCallbackFree(callback) isn't called >> and we leak callback. At least that's how I read it > > First - sorry for the late response! > > The unregister/free’ing is either done within the > 'remoteClientFreePrivateCallbacks' call or by an explicit call of > 'remoteDispatchConnectUnregisterCloseCallback'. Do we agree on this? If > yes: the function 'remoteClientFreePrivateCallbacks' calls the > virConnectUnregisterCloseCallback -> remoteEventCallbackFree => no > memory leak. > > There will be only a memory leak if the virConnectRegisterCloseCallback > call succeeds but the used driver had no support for registering a close > callback (conn->driver->connectRegisterCloseCallback was NULL) AND the > first patch of this series were not applied. > > Did I miss something? > Trying to recollect where I was.... and I cannot... I probably was flipping between patched and unpatched code. I assume it had something to do with adding the callback to a list, running DEREG_CB processing, and perhaps seeing DELETE_ELEMENT that got me overthinking, but taking a second look now I don't believe there's an issue. So, to make it official then... Reviewed-by: John Ferlan <jferlan@redhat.com> John -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
© 2016 - 2026 Red Hat, Inc.