[Qemu-devel] [PATCH] vl: fix resource leak with monitor_fdset_add_fd

[Qemu-devel] [PATCH] vl: fix resource leak with monitor_fdset_add_fd

[Paolo Bonzini]

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>
---
 vl.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/vl.c b/vl.c
index eba5d4c..8902435 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,9 @@ 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, NULL);
+    g_free(fdinfo);
 
     return 0;
 }
-- 
2.3.0

Re: [Qemu-devel] [PATCH] vl: fix resource leak with monitor_fdset_add_fd

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 440 bytes --]

On 03/12/2015 08:57 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>
> ---
>  vl.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)

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

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH] vl: fix resource leak with monitor_fdset_add_fd

[Fam Zheng]

On Thu, 03/12 15:57, 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>
> ---
>  vl.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/vl.c b/vl.c
> index eba5d4c..8902435 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,9 @@ 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,

Too long a line, with that fixed,

Reviewed-by: Fam Zheng <famz@redhat.com>

> +                                  fd_opaque, NULL);
> +    g_free(fdinfo);
>  
>      return 0;
>  }
> -- 
> 2.3.0
> 
> 

Re: [Qemu-devel] [PATCH] vl: fix resource leak with monitor_fdset_add_fd

[Patchew Tool]

This series passed Patchew automatic testing, but there are some warnings.

Find the log fragments below, or open the following URL to see the full log:

http://qemu.patchew.org/testing/log/<1426172243-7830-1-git-send-email-pbonzini@redhat.com>

----------8<---------


=== Starting docker ===

Copying test script "/home/patchew/tests/qemu-devel.sh"
Starting docker...
docker run --net=none -v /var/tmp/patchew-tester/tmpELr8N5:/var/tmp/patchew-test patchew:fedora-20 timeout 3600 /var/tmp/patchew-test/qemu-devel.sh /var/tmp/patchew-test

*** Testing 'coding style check' ***

Checking 0001-vl--fix-resource-leak-with-monitor_fdset_add_fd.patch
command failed with exit code 0
$@
WARNING: line over 80 characters
#27: FILE: vl.c:1064:
+    fdinfo = monitor_fdset_add_fd(dupfd, true, fdset_id, fd_opaque ? true : false,

total: 0 errors, 1 warnings, 18 lines checked

0001-vl--fix-resource-leak-with-monitor_fdset_add_fd.patch has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.


*** Testing 'configure' ***

Install prefix    /usr/local
BIOS directory    /usr/local/share/qemu
binary directory  /usr/local/bin
library directory /usr/local/lib
module directory  /usr/local/lib/qemu
libexec directory /usr/local/libexec

Re: [Qemu-devel] [PATCH] vl: fix resource leak with monitor_fdset_add_fd

[Shannon Zhao]

On 2015/3/12 22:57, 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>
> ---
>  vl.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/vl.c b/vl.c
> index eba5d4c..8902435 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,9 @@ 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, NULL);
> +    g_free(fdinfo);
>  
>      return 0;
>  }
> 

Hi Paolo,

Not about the resource leak, but I think we should check fdinfo == NULL and return -1.
Then when error occurs, qemu will exit. Besides we should pass a meaningful errp not NULL.
So qemu can report the error reason. Right?

-- 
Thanks,
Shannon

Re: [Qemu-devel] [PATCH] vl: fix resource leak with monitor_fdset_add_fd

[Paolo Bonzini]

On 13/03/2015 06:17, Shannon Zhao wrote:
>> > -    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, NULL);
>> > +    g_free(fdinfo);
>> >  
>> >      return 0;
>> >  }
>> > 
> Hi Paolo,
> 
> Not about the resource leak, but I think we should check fdinfo == NULL and return -1.
> Then when error occurs, qemu will exit. Besides we should pass a meaningful errp not NULL.
> So qemu can report the error reason. Right?

It cannot fail here, so another possibility could be to pass &error_abort.

Paolo

Re: [Qemu-devel] [PATCH] vl: fix resource leak with monitor_fdset_add_fd

[Michael Tokarev]

12.03.2015 17:57, Paolo Bonzini wrote:
> monitor_fdset_add_fd returns an AddfdInfo struct (used by the QMP
> command add_fd).  Free it.

Applied to -trivial, with a tiny change:

> +    fdinfo = 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, NULL);

(this fits in 80 chars a line).  Another variant is to use `fd_opaque != NULL' here
instead of this ?true:false construct.

Thanks,

/mjt

Re: [Qemu-devel] [PATCH] vl: fix resource leak with monitor_fdset_add_fd

[Markus Armbruster]

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.

Re: [Qemu-devel] [PATCH] vl: fix resource leak with monitor_fdset_add_fd

[Shannon Zhao]

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;
>  }
> 


-- 
Thanks,
Shannon

Re: [Qemu-devel] [PATCH] vl: fix resource leak with monitor_fdset_add_fd

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1491 bytes --]

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.


-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

[Qemu-devel] [PATCH] vl: fix resource leak with monitor_fdset_add_fd

[Paolo Bonzini]

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(-)

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;
 }
-- 
2.3.0