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 9580e854efbe..95940ffdeefe 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);
@@ -3814,6 +3814,7 @@ remoteDispatchConnectRegisterCloseCallback(virNetServerPtr server ATTRIBUTE_UNUS
virNetMessageErrorPtr rerr)
{
int rv = -1;
+ daemonClientEventCallbackPtr callback = NULL;
struct daemonClientPrivate *priv =
virNetServerClientGetPrivateData(client);
@@ -3824,9 +3825,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;
@@ -3834,8 +3842,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 03/08/2018 07:20 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(-)
>
Reviewed-by: John Ferlan <jferlan@redhat.com>
John
(see my note below, I imagine you agree...)
> diff --git a/src/remote/remote_daemon_dispatch.c b/src/remote/remote_daemon_dispatch.c
> index 9580e854efbe..95940ffdeefe 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);
> @@ -3814,6 +3814,7 @@ remoteDispatchConnectRegisterCloseCallback(virNetServerPtr server ATTRIBUTE_UNUS
> virNetMessageErrorPtr rerr)
> {
> int rv = -1;
> + daemonClientEventCallbackPtr callback = NULL;
> struct daemonClientPrivate *priv =
> virNetServerClientGetPrivateData(client);
>
> @@ -3824,9 +3825,16 @@ remoteDispatchConnectRegisterCloseCallback(virNetServerPtr server ATTRIBUTE_UNUS
> goto cleanup;
> }
>
> + if (VIR_ALLOC(callback) < 0)
> + goto cleanup;
> + callback->client = virObjectRef(client);
This one's scary because currently that means we currently call
virConnectRegisterCloseCallback, but haven't been doing the
virObjectRef(client) prior to using it as an opaque... Meaning
remoteRelayConnectionClosedEvent could be called with client already free'd.
> + 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;
> @@ -3834,8 +3842,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, Mar 15, 2018 at 07:56 PM +0100, John Ferlan <jferlan@redhat.com> wrote:
> On 03/08/2018 07:20 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(-)
>>
>
> Reviewed-by: John Ferlan <jferlan@redhat.com>
>
> John
Thanks.
>
> (see my note below, I imagine you agree...)
>
>> diff --git a/src/remote/remote_daemon_dispatch.c b/src/remote/remote_daemon_dispatch.c
>> index 9580e854efbe..95940ffdeefe 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);
>> @@ -3814,6 +3814,7 @@ remoteDispatchConnectRegisterCloseCallback(virNetServerPtr server ATTRIBUTE_UNUS
>> virNetMessageErrorPtr rerr)
>> {
>> int rv = -1;
>> + daemonClientEventCallbackPtr callback = NULL;
>> struct daemonClientPrivate *priv =
>> virNetServerClientGetPrivateData(client);
>>
>> @@ -3824,9 +3825,16 @@ remoteDispatchConnectRegisterCloseCallback(virNetServerPtr server ATTRIBUTE_UNUS
>> goto cleanup;
>> }
>>
>> + if (VIR_ALLOC(callback) < 0)
>> + goto cleanup;
>> + callback->client = virObjectRef(client);
>
> This one's scary because currently that means we currently call
> virConnectRegisterCloseCallback, but haven't been doing the
> virObjectRef(client) prior to using it as an opaque... Meaning
> remoteRelayConnectionClosedEvent could be called with client already
> free'd.
Yep. Or at least, I think so.
>
>
>> + 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;
>> @@ -3834,8 +3842,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 Thu, Mar 15, 2018 at 07:56 PM +0100, John Ferlan <jferlan@redhat.com> wrote:
> On 03/08/2018 07:20 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(-)
>>
>
> Reviewed-by: John Ferlan <jferlan@redhat.com>
>
> John
>
> (see my note below, I imagine you agree...)
>
>> diff --git a/src/remote/remote_daemon_dispatch.c b/src/remote/remote_daemon_dispatch.c
>> index 9580e854efbe..95940ffdeefe 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);
>> @@ -3814,6 +3814,7 @@ remoteDispatchConnectRegisterCloseCallback(virNetServerPtr server ATTRIBUTE_UNUS
>> virNetMessageErrorPtr rerr)
>> {
>> int rv = -1;
>> + daemonClientEventCallbackPtr callback = NULL;
>> struct daemonClientPrivate *priv =
>> virNetServerClientGetPrivateData(client);
>>
>> @@ -3824,9 +3825,16 @@ remoteDispatchConnectRegisterCloseCallback(virNetServerPtr server ATTRIBUTE_UNUS
>> goto cleanup;
>> }
>>
>> + if (VIR_ALLOC(callback) < 0)
>> + goto cleanup;
>> + callback->client = virObjectRef(client);
>
> This one's scary because currently that means we currently call
> virConnectRegisterCloseCallback, but haven't been doing the
> virObjectRef(client) prior to using it as an opaque... Meaning
> remoteRelayConnectionClosedEvent could be called with client already free'd.
>
>
>> + 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;
This is where I produced a memory leak, relying on
virConnectRegisterCloseCallback to return < 0 if something failed.
>>
>> priv->closeRegistered = true;
>> @@ -3834,8 +3842,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 15.03.2018 21:56, John Ferlan wrote:
>
>
> On 03/08/2018 07:20 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(-)
>>
>
> Reviewed-by: John Ferlan <jferlan@redhat.com>
>
> John
>
> (see my note below, I imagine you agree...)
>
>> diff --git a/src/remote/remote_daemon_dispatch.c b/src/remote/remote_daemon_dispatch.c
>> index 9580e854efbe..95940ffdeefe 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);
>> @@ -3814,6 +3814,7 @@ remoteDispatchConnectRegisterCloseCallback(virNetServerPtr server ATTRIBUTE_UNUS
>> virNetMessageErrorPtr rerr)
>> {
>> int rv = -1;
>> + daemonClientEventCallbackPtr callback = NULL;
>> struct daemonClientPrivate *priv =
>> virNetServerClientGetPrivateData(client);
>>
>> @@ -3824,9 +3825,16 @@ remoteDispatchConnectRegisterCloseCallback(virNetServerPtr server ATTRIBUTE_UNUS
>> goto cleanup;
>> }
>>
>> + if (VIR_ALLOC(callback) < 0)
>> + goto cleanup;
>> + callback->client = virObjectRef(client);
>
> This one's scary because currently that means we currently call
> virConnectRegisterCloseCallback, but haven't been doing the
> virObjectRef(client) prior to using it as an opaque... Meaning
> remoteRelayConnectionClosedEvent could be called with client already free'd.
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 :).
ce35122cfe: "daemon: fixup refcounting in close callback handling"
Nikolay
>
>
>> + 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;
>> @@ -3834,8 +3842,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
>
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
© 2016 - 2025 Red Hat, Inc.