[Qemu-devel] [PATCH] compatfd.c: Don't pass NULL pointer to SYS_signalfd

[Qemu-devel] [PATCH] compatfd.c: Don't pass NULL pointer to SYS_signalfd

[Peter Maydell]

Don't pass a NULL pointer in to SYS_signalfd in qemu_signalfd_available():
this isn't valid and Valgrind complains about it.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 compatfd.c |   12 ++++++++++--
 1 files changed, 10 insertions(+), 2 deletions(-)

diff --git a/compatfd.c b/compatfd.c
index 31654c6..02306a4 100644
--- a/compatfd.c
+++ b/compatfd.c
@@ -119,9 +119,17 @@ int qemu_signalfd(const sigset_t *mask)
 bool qemu_signalfd_available(void)
 {
 #ifdef CONFIG_SIGNALFD
+    sigset_t mask;
+    int fd;
+    bool ok;
+    sigemptyset(&mask);
     errno = 0;
-    syscall(SYS_signalfd, -1, NULL, _NSIG / 8);
-    return errno != ENOSYS;
+    fd = syscall(SYS_signalfd, -1, &mask, _NSIG / 8);
+    ok = (errno != ENOSYS);
+    if (fd >= 0) {
+        close(fd);
+    }
+    return ok;
 #else
     return false;
 #endif
-- 
1.7.1

Re: [Qemu-devel] [PATCH] compatfd.c: Don't pass NULL pointer to SYS_signalfd

[Stefan Hajnoczi]

On Thu, Oct 13, 2011 at 06:45:37PM +0100, Peter Maydell wrote:
> Don't pass a NULL pointer in to SYS_signalfd in qemu_signalfd_available():
> this isn't valid and Valgrind complains about it.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  compatfd.c |   12 ++++++++++--
>  1 files changed, 10 insertions(+), 2 deletions(-)

Reviewed-by: Stefan Hajnoczi <stefanha@linux.vnet.ibm.com>

Re: [Qemu-devel] [PATCH] compatfd.c: Don't pass NULL pointer to SYS_signalfd

[Stefan Weil]

Am 13.10.2011 19:45, schrieb Peter Maydell:
> Don't pass a NULL pointer in to SYS_signalfd in qemu_signalfd_available():
> this isn't valid and Valgrind complains about it.
>
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
> compatfd.c | 12 ++++++++++--
> 1 files changed, 10 insertions(+), 2 deletions(-)
>
> diff --git a/compatfd.c b/compatfd.c
> index 31654c6..02306a4 100644
> --- a/compatfd.c
> +++ b/compatfd.c
> @@ -119,9 +119,17 @@ int qemu_signalfd(const sigset_t *mask)
> bool qemu_signalfd_available(void)
> {
> #ifdef CONFIG_SIGNALFD
> + sigset_t mask;
> + int fd;
> + bool ok;
> + sigemptyset(&mask);
> errno = 0;
> - syscall(SYS_signalfd, -1, NULL, _NSIG / 8);
> - return errno != ENOSYS;
> + fd = syscall(SYS_signalfd, -1, &mask, _NSIG / 8);
> + ok = (errno != ENOSYS);
> + if (fd >= 0) {

Maybe better: fd != -1

> + close(fd);
> + }
> + return ok;
> #else
> return false;
> #endif

The variable 'ok' is not needed, simply returning
errno != ENOSYS would work, too.

Regards,
Stefan W.

Re: [Qemu-devel] [PATCH] compatfd.c: Don't pass NULL pointer to SYS_signalfd

[Peter Maydell]

On 14 October 2011 18:20, Stefan Weil <sw@weilnetz.de> wrote:
> Am 13.10.2011 19:45, schrieb Peter Maydell:
>>
>> Don't pass a NULL pointer in to SYS_signalfd in qemu_signalfd_available():
>> this isn't valid and Valgrind complains about it.
>>
>> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
>> ---
>> compatfd.c | 12 ++++++++++--
>> 1 files changed, 10 insertions(+), 2 deletions(-)
>>
>> diff --git a/compatfd.c b/compatfd.c
>> index 31654c6..02306a4 100644
>> --- a/compatfd.c
>> +++ b/compatfd.c
>> @@ -119,9 +119,17 @@ int qemu_signalfd(const sigset_t *mask)
>> bool qemu_signalfd_available(void)
>> {
>> #ifdef CONFIG_SIGNALFD
>> + sigset_t mask;
>> + int fd;
>> + bool ok;
>> + sigemptyset(&mask);
>> errno = 0;
>> - syscall(SYS_signalfd, -1, NULL, _NSIG / 8);
>> - return errno != ENOSYS;
>> + fd = syscall(SYS_signalfd, -1, &mask, _NSIG / 8);
>> + ok = (errno != ENOSYS);
>> + if (fd >= 0) {
>
> Maybe better: fd != -1

Style issue -- I prefer the >= 0; if you do a
'git grep -A2 open' you'll see that mostly the existing
codebase does 'is it less than zero or not' comparisons
for "did this thing returning an fd fail?" checks, rather
than 'is it equal to -1 or not'.

>> + close(fd);
>> + }
>> + return ok;
>> #else
>> return false;
>> #endif
>
> The variable 'ok' is not needed, simply returning
> errno != ENOSYS would work, too.

The call to close() might have trashed errno (although
admittedly the chances of close() returning ENOSYS are
rather low I think it's clearer to return the result
of checking the errno for the syscall we care about rather
than the one we don't).

-- PMM

Re: [Qemu-devel] [PATCH] compatfd.c: Don't pass NULL pointer to SYS_signalfd

[andrzej zaborowski]

On 13 October 2011 19:45, Peter Maydell <peter.maydell@linaro.org> wrote:
> Don't pass a NULL pointer in to SYS_signalfd in qemu_signalfd_available():
> this isn't valid and Valgrind complains about it.

Also pushed this patch.

Cheers