[Qemu-devel] [PATCH] file-posix: Don't leak fd in hdev_get_max_segments

[Qemu-devel] [PATCH] file-posix: Don't leak fd in hdev_get_max_segments

[Fam Zheng]

Signed-off-by: Fam Zheng <famz@redhat.com>
---
 block/file-posix.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/block/file-posix.c b/block/file-posix.c
index c4c0663..e6170f4 100644
--- a/block/file-posix.c
+++ b/block/file-posix.c
@@ -703,6 +703,7 @@ static int hdev_get_max_segments(const struct stat *st)
     }
 
 out:
+    close(fd);
     g_free(sysfspath);
     return ret;
 #else
-- 
2.9.3

Re: [Qemu-devel] [PATCH] file-posix: Don't leak fd in hdev_get_max_segments

[Kevin Wolf]

Am 14.03.2017 um 16:43 hat Fam Zheng geschrieben:
> Signed-off-by: Fam Zheng <famz@redhat.com>
> ---
>  block/file-posix.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/block/file-posix.c b/block/file-posix.c
> index c4c0663..e6170f4 100644
> --- a/block/file-posix.c
> +++ b/block/file-posix.c
> @@ -703,6 +703,7 @@ static int hdev_get_max_segments(const struct stat *st)
>      }
>  
>  out:
> +    close(fd);

Should we make this conditional on fd != -1?

>      g_free(sysfspath);
>      return ret;
>  #else

Kevin

Re: [Qemu-devel] [PATCH] file-posix: Don't leak fd in hdev_get_max_segments

[Fam Zheng]

On Tue, 03/14 16:50, Kevin Wolf wrote:
> Am 14.03.2017 um 16:43 hat Fam Zheng geschrieben:
> > Signed-off-by: Fam Zheng <famz@redhat.com>
> > ---
> >  block/file-posix.c | 1 +
> >  1 file changed, 1 insertion(+)
> > 
> > diff --git a/block/file-posix.c b/block/file-posix.c
> > index c4c0663..e6170f4 100644
> > --- a/block/file-posix.c
> > +++ b/block/file-posix.c
> > @@ -703,6 +703,7 @@ static int hdev_get_max_segments(const struct stat *st)
> >      }
> >  
> >  out:
> > +    close(fd);
> 
> Should we make this conditional on fd != -1?

OK, that is cleaner.

Fam

> 
> >      g_free(sysfspath);
> >      return ret;
> >  #else
> 
> Kevin

Re: [Qemu-devel] [PATCH] file-posix: Don't leak fd in hdev_get_max_segments

[Eric Blake]

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

On 03/14/2017 10:43 AM, Fam Zheng wrote:
> Signed-off-by: Fam Zheng <famz@redhat.com>
> ---
>  block/file-posix.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/block/file-posix.c b/block/file-posix.c
> index c4c0663..e6170f4 100644
> --- a/block/file-posix.c
> +++ b/block/file-posix.c
> @@ -703,6 +703,7 @@ static int hdev_get_max_segments(const struct stat *st)

hdev_get_max_segments() is not part of master yet; should this just be
treated as a fixup! to a pending patch?

>      }
>  
>  out:
> +    close(fd);
>      g_free(sysfspath);
>      return ret;
>  #else
> 

-- 
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] file-posix: Don't leak fd in hdev_get_max_segments

[Eric Blake]

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

On 03/14/2017 11:11 AM, Eric Blake wrote:
> On 03/14/2017 10:43 AM, Fam Zheng wrote:
>> Signed-off-by: Fam Zheng <famz@redhat.com>
>> ---
>>  block/file-posix.c | 1 +
>>  1 file changed, 1 insertion(+)
>>
>> diff --git a/block/file-posix.c b/block/file-posix.c
>> index c4c0663..e6170f4 100644
>> --- a/block/file-posix.c
>> +++ b/block/file-posix.c
>> @@ -703,6 +703,7 @@ static int hdev_get_max_segments(const struct stat *st)
> 
> hdev_get_max_segments() is not part of master yet; should this just be
> treated as a fixup! to a pending patch?

Scratch that - I forgot to 'git pull' on my end.

Commit message could usefully be improved by mentioning commit 9103f1ce
as the spot the problem was introduced.

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] file-posix: Don't leak fd in hdev_get_max_segments

[Eric Blake]

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

On 03/14/2017 11:14 AM, Eric Blake wrote:
> On 03/14/2017 11:11 AM, Eric Blake wrote:
>> On 03/14/2017 10:43 AM, Fam Zheng wrote:
>>> Signed-off-by: Fam Zheng <famz@redhat.com>
>>> ---
>>>  block/file-posix.c | 1 +
>>>  1 file changed, 1 insertion(+)
>>>
>>> diff --git a/block/file-posix.c b/block/file-posix.c
>>> index c4c0663..e6170f4 100644
>>> --- a/block/file-posix.c
>>> +++ b/block/file-posix.c
>>> @@ -703,6 +703,7 @@ static int hdev_get_max_segments(const struct stat *st)
>>
>> hdev_get_max_segments() is not part of master yet; should this just be
>> treated as a fixup! to a pending patch?
> 
> Scratch that - I forgot to 'git pull' on my end.
> 
> Commit message could usefully be improved by mentioning commit 9103f1ce
> as the spot the problem was introduced.
> 
> Reviewed-by: Eric Blake <eblake@redhat.com>
> 

Question: does valgrind or any other tool complain about:

+    if (fd == -1) {
+        ret = -errno;
+        goto out;
+    }
...
+out:
+    close(fd);
+    g_free(sysfspath);
+    return ret;

this being a useless syscall to close(-1) ?  If so, you need to wrap in
an 'if (fd != -1)'

-- 
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] file-posix: Don't leak fd in hdev_get_max_segments

[Kevin Wolf]

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

Am 14.03.2017 um 17:11 hat Eric Blake geschrieben:
> On 03/14/2017 10:43 AM, Fam Zheng wrote:
> > Signed-off-by: Fam Zheng <famz@redhat.com>
> > ---
> >  block/file-posix.c | 1 +
> >  1 file changed, 1 insertion(+)
> > 
> > diff --git a/block/file-posix.c b/block/file-posix.c
> > index c4c0663..e6170f4 100644
> > --- a/block/file-posix.c
> > +++ b/block/file-posix.c
> > @@ -703,6 +703,7 @@ static int hdev_get_max_segments(const struct stat *st)
> 
> hdev_get_max_segments() is not part of master yet; should this just be
> treated as a fixup! to a pending patch?

It is already in master, since yesterday, I think.

Kevin

[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]