[libvirt] [PATCH] event: reference state only when virEventAddTimeout success

Xu Yandong posted 1 patch 4 years, 6 months ago
Test syntax-check passed
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/libvirt tags/patchew/1568880135-88269-1-git-send-email-xuyandong2@huawei.com
src/conf/object_event.c | 25 +++++++++++++------------
1 file changed, 13 insertions(+), 12 deletions(-)
[libvirt] [PATCH] event: reference state only when virEventAddTimeout success
Posted by Xu Yandong 4 years, 6 months ago
Reference state is not necessary when virEventAddTimeout failed,
this may cause a memory leak, so reference state only when
virEventAddTimeout success.

Signed-off-by: Xu Yandong <xuyandong2@huawei.com>
---
 src/conf/object_event.c | 25 +++++++++++++------------
 1 file changed, 13 insertions(+), 12 deletions(-)

diff --git a/src/conf/object_event.c b/src/conf/object_event.c
index 5d84598d59..ee5def5910 100644
--- a/src/conf/object_event.c
+++ b/src/conf/object_event.c
@@ -891,20 +891,21 @@ virObjectEventStateRegisterID(virConnectPtr conn,
     virObjectLock(state);
 
     if ((state->callbacks->count == 0) &&
-        (state->timer == -1) &&
-        (state->timer = virEventAddTimeout(-1,
-                                           virObjectEventTimer,
-                                           state,
-                                           virObjectFreeCallback)) < 0) {
-        virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
-                       _("could not initialize domain event timer"));
-        goto cleanup;
+        (state->timer == -1)) {
+        if ((state->timer = virEventAddTimeout(-1,
+                                               virObjectEventTimer,
+                                               state,
+                                               virObjectFreeCallback)) < 0) {
+            virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+                           _("could not initialize domain event timer"));
+            goto cleanup;
+        } else {
+            /* event loop has one reference, but we need one more for the
+             * timer's opaque argument */
+            virObjectRef(state);
+        }
     }
 
-    /* event loop has one reference, but we need one more for the
-     * timer's opaque argument */
-    virObjectRef(state);
-
     ret = virObjectEventCallbackListAddID(conn, state->callbacks,
                                           key, filter, filter_opaque,
                                           klass, eventID,
-- 
2.18.1

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] event: reference state only when virEventAddTimeout success
Posted by Daniel Henrique Barboza 4 years, 6 months ago

On 9/19/19 5:02 AM, Xu Yandong wrote:
> Reference state is not necessary when virEventAddTimeout failed,
> this may cause a memory leak, so reference state only when
> virEventAddTimeout success.
>
> Signed-off-by: Xu Yandong <xuyandong2@huawei.com>
> ---

Reviewed-by: Daniel Henrique Barboza <danielhb413@gmail.com>

>   src/conf/object_event.c | 25 +++++++++++++------------
>   1 file changed, 13 insertions(+), 12 deletions(-)
>
> diff --git a/src/conf/object_event.c b/src/conf/object_event.c
> index 5d84598d59..ee5def5910 100644
> --- a/src/conf/object_event.c
> +++ b/src/conf/object_event.c
> @@ -891,20 +891,21 @@ virObjectEventStateRegisterID(virConnectPtr conn,
>       virObjectLock(state);
>   
>       if ((state->callbacks->count == 0) &&
> -        (state->timer == -1) &&
> -        (state->timer = virEventAddTimeout(-1,
> -                                           virObjectEventTimer,
> -                                           state,
> -                                           virObjectFreeCallback)) < 0) {
> -        virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> -                       _("could not initialize domain event timer"));
> -        goto cleanup;
> +        (state->timer == -1)) {
> +        if ((state->timer = virEventAddTimeout(-1,
> +                                               virObjectEventTimer,
> +                                               state,
> +                                               virObjectFreeCallback)) < 0) {
> +            virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> +                           _("could not initialize domain event timer"));
> +            goto cleanup;
> +        } else {
> +            /* event loop has one reference, but we need one more for the
> +             * timer's opaque argument */
> +            virObjectRef(state);
> +        }
>       }
>   
> -    /* event loop has one reference, but we need one more for the
> -     * timer's opaque argument */
> -    virObjectRef(state);
> -
>       ret = virObjectEventCallbackListAddID(conn, state->callbacks,
>                                             key, filter, filter_opaque,
>                                             klass, eventID,

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] event: reference state only when virEventAddTimeout success
Posted by Michal Privoznik 4 years, 6 months ago
On 9/19/19 10:02 AM, Xu Yandong wrote:
> Reference state is not necessary when virEventAddTimeout failed,
> this may cause a memory leak, so reference state only when
> virEventAddTimeout success.
> 
> Signed-off-by: Xu Yandong <xuyandong2@huawei.com>
> ---
>   src/conf/object_event.c | 25 +++++++++++++------------
>   1 file changed, 13 insertions(+), 12 deletions(-)
> 
> diff --git a/src/conf/object_event.c b/src/conf/object_event.c
> index 5d84598d59..ee5def5910 100644
> --- a/src/conf/object_event.c
> +++ b/src/conf/object_event.c
> @@ -891,20 +891,21 @@ virObjectEventStateRegisterID(virConnectPtr conn,
>       virObjectLock(state);
>   
>       if ((state->callbacks->count == 0) &&
> -        (state->timer == -1) &&
> -        (state->timer = virEventAddTimeout(-1,
> -                                           virObjectEventTimer,
> -                                           state,
> -                                           virObjectFreeCallback)) < 0) {
> -        virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> -                       _("could not initialize domain event timer"));
> -        goto cleanup;
> +        (state->timer == -1)) {
> +        if ((state->timer = virEventAddTimeout(-1,
> +                                               virObjectEventTimer,
> +                                               state,
> +                                               virObjectFreeCallback)) < 0) {
> +            virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> +                           _("could not initialize domain event timer"));
> +            goto cleanup;
> +        } else {
> +            /* event loop has one reference, but we need one more for the
> +             * timer's opaque argument */
> +            virObjectRef(state);
> +        }

This doesn't have to be under else branch since there's no way the 
control can return from the above branch.

>       }
>   
> -    /* event loop has one reference, but we need one more for the
> -     * timer's opaque argument */
> -    virObjectRef(state);
> -
>       ret = virObjectEventCallbackListAddID(conn, state->callbacks,
>                                             key, filter, filter_opaque,
>                                             klass, eventID,
> 

I'm rewording the commit message a bit, ACKing and pushing.

Congratulations on your first libvirt contribution.

Michal

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list