diff mbox

vl: fix resource leak with monitor_fdset_add_fd

Message ID 1426251334-29445-1-git-send-email-pbonzini@redhat.com
State New
Headers show

Commit Message

Paolo Bonzini March 13, 2015, 12:55 p.m. UTC
monitor_fdset_add_fd returns an AddfdInfo struct (used by the QMP
command add_fd).  Free it.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
	v1->v2: line length [Fam], pass &error_abort [Shannon]
---
 vl.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

Comments

Eric Blake March 13, 2015, 1:56 p.m. UTC | #1
On 03/13/2015 06:55 AM, Paolo Bonzini wrote:
> monitor_fdset_add_fd returns an AddfdInfo struct (used by the QMP
> command add_fd).  Free it.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
> 	v1->v2: line length [Fam], pass &error_abort [Shannon]
> ---
>  vl.c | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
> 

Reviewed-by: Eric Blake <eblake@redhat.com>

> diff --git a/vl.c b/vl.c
> index eba5d4c..9eae8f9 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -1011,6 +1011,7 @@ static int parse_add_fd(QemuOpts *opts, void *opaque)
>      int fd, dupfd, flags;
>      int64_t fdset_id;
>      const char *fd_opaque = NULL;
> +    AddfdInfo *fdinfo;
>  
>      fd = qemu_opt_get_number(opts, "fd", -1);
>      fdset_id = qemu_opt_get_number(opts, "set", -1);
> @@ -1060,8 +1061,10 @@ static int parse_add_fd(QemuOpts *opts, void *opaque)
>      }
>  
>      /* add the duplicate fd, and optionally the opaque string, to the fd set */
> -    monitor_fdset_add_fd(dupfd, true, fdset_id, fd_opaque ? true : false,
> -                         fd_opaque, NULL);
> +    fdinfo = monitor_fdset_add_fd(dupfd, true, fdset_id,
> +                                  fd_opaque ? true : false, fd_opaque,

I might have written !!fd_opaque (as cond ? true : false always looks so
long), but that's cosmetic and doesn't affect the review.
Shannon Zhao March 14, 2015, 1:15 a.m. UTC | #2
On 2015/3/13 20:55, Paolo Bonzini wrote:
> monitor_fdset_add_fd returns an AddfdInfo struct (used by the QMP
> command add_fd).  Free it.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
> 	v1->v2: line length [Fam], pass &error_abort [Shannon]
> ---
>  vl.c | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
> 

Reviewed-by: Shannon Zhao <shannon.zhao@linaro.org>

> diff --git a/vl.c b/vl.c
> index eba5d4c..9eae8f9 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -1011,6 +1011,7 @@ static int parse_add_fd(QemuOpts *opts, void *opaque)
>      int fd, dupfd, flags;
>      int64_t fdset_id;
>      const char *fd_opaque = NULL;
> +    AddfdInfo *fdinfo;
>  
>      fd = qemu_opt_get_number(opts, "fd", -1);
>      fdset_id = qemu_opt_get_number(opts, "set", -1);
> @@ -1060,8 +1061,10 @@ static int parse_add_fd(QemuOpts *opts, void *opaque)
>      }
>  
>      /* add the duplicate fd, and optionally the opaque string, to the fd set */
> -    monitor_fdset_add_fd(dupfd, true, fdset_id, fd_opaque ? true : false,
> -                         fd_opaque, NULL);
> +    fdinfo = monitor_fdset_add_fd(dupfd, true, fdset_id,
> +                                  fd_opaque ? true : false, fd_opaque,
> +                                  &error_abort);
> +    g_free(fdinfo);
>  
>      return 0;
>  }
>
Markus Armbruster March 14, 2015, 8:08 a.m. UTC | #3
Eric Blake <eblake@redhat.com> writes:

> On 03/13/2015 06:55 AM, Paolo Bonzini wrote:
>> monitor_fdset_add_fd returns an AddfdInfo struct (used by the QMP
>> command add_fd).  Free it.
>> 
>> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
>> ---
>> 	v1->v2: line length [Fam], pass &error_abort [Shannon]
>> ---
>>  vl.c | 7 +++++--
>>  1 file changed, 5 insertions(+), 2 deletions(-)
>> 
>
> Reviewed-by: Eric Blake <eblake@redhat.com>
>
>> diff --git a/vl.c b/vl.c
>> index eba5d4c..9eae8f9 100644
>> --- a/vl.c
>> +++ b/vl.c
>> @@ -1011,6 +1011,7 @@ static int parse_add_fd(QemuOpts *opts, void *opaque)
>>      int fd, dupfd, flags;
>>      int64_t fdset_id;
>>      const char *fd_opaque = NULL;
>> +    AddfdInfo *fdinfo;
>>  
>>      fd = qemu_opt_get_number(opts, "fd", -1);
>>      fdset_id = qemu_opt_get_number(opts, "set", -1);
>> @@ -1060,8 +1061,10 @@ static int parse_add_fd(QemuOpts *opts, void *opaque)
>>      }
>>  
>>      /* add the duplicate fd, and optionally the opaque string, to
>> the fd set */
>> -    monitor_fdset_add_fd(dupfd, true, fdset_id, fd_opaque ? true : false,
>> -                         fd_opaque, NULL);
>> +    fdinfo = monitor_fdset_add_fd(dupfd, true, fdset_id,
>> +                                  fd_opaque ? true : false, fd_opaque,
>
> I might have written !!fd_opaque (as cond ? true : false always looks so
> long), but that's cosmetic and doesn't affect the review.

The use of ?: here borders on code obfuscation.  Please clean it up.

If you can't stand !!fd_opaque, then use fd_opaque != NULL.
diff mbox

Patch

diff --git a/vl.c b/vl.c
index eba5d4c..9eae8f9 100644
--- a/vl.c
+++ b/vl.c
@@ -1011,6 +1011,7 @@  static int parse_add_fd(QemuOpts *opts, void *opaque)
     int fd, dupfd, flags;
     int64_t fdset_id;
     const char *fd_opaque = NULL;
+    AddfdInfo *fdinfo;
 
     fd = qemu_opt_get_number(opts, "fd", -1);
     fdset_id = qemu_opt_get_number(opts, "set", -1);
@@ -1060,8 +1061,10 @@  static int parse_add_fd(QemuOpts *opts, void *opaque)
     }
 
     /* add the duplicate fd, and optionally the opaque string, to the fd set */
-    monitor_fdset_add_fd(dupfd, true, fdset_id, fd_opaque ? true : false,
-                         fd_opaque, NULL);
+    fdinfo = monitor_fdset_add_fd(dupfd, true, fdset_id,
+                                  fd_opaque ? true : false, fd_opaque,
+                                  &error_abort);
+    g_free(fdinfo);
 
     return 0;
 }