* [Qemu-devel] [PATCH] tests/hd-geo-test: Don't pass NULL to unlink()
@ 2016-08-05 10:03 Peter Maydell
2016-08-05 11:19 ` Markus Armbruster
` (2 more replies)
0 siblings, 3 replies; 9+ messages in thread
From: Peter Maydell @ 2016-08-05 10:03 UTC (permalink / raw)
To: qemu-devel; +Cc: patches
The unlink() function doesn't accept a NULL pointer, so
don't pass it one. Spotted by the clang sanitizer.
Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
tests/hd-geo-test.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/tests/hd-geo-test.c b/tests/hd-geo-test.c
index 12ee392..6176e81 100644
--- a/tests/hd-geo-test.c
+++ b/tests/hd-geo-test.c
@@ -416,7 +416,9 @@ int main(int argc, char **argv)
ret = g_test_run();
for (i = 0; i < backend_last; i++) {
- unlink(img_file_name[i]);
+ if (img_file_name[i]) {
+ unlink(img_file_name[i]);
+ }
}
return ret;
--
2.7.4
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [Qemu-devel] [PATCH] tests/hd-geo-test: Don't pass NULL to unlink()
2016-08-05 10:03 [Qemu-devel] [PATCH] tests/hd-geo-test: Don't pass NULL to unlink() Peter Maydell
@ 2016-08-05 11:19 ` Markus Armbruster
2016-08-05 11:57 ` Peter Maydell
2016-09-06 12:47 ` Peter Maydell
2016-09-08 10:27 ` Peter Maydell
2 siblings, 1 reply; 9+ messages in thread
From: Markus Armbruster @ 2016-08-05 11:19 UTC (permalink / raw)
To: Peter Maydell; +Cc: qemu-devel, patches
Peter Maydell <peter.maydell@linaro.org> writes:
> The unlink() function doesn't accept a NULL pointer, so
> don't pass it one. Spotted by the clang sanitizer.
>
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
> tests/hd-geo-test.c | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/tests/hd-geo-test.c b/tests/hd-geo-test.c
> index 12ee392..6176e81 100644
> --- a/tests/hd-geo-test.c
> +++ b/tests/hd-geo-test.c
> @@ -416,7 +416,9 @@ int main(int argc, char **argv)
> ret = g_test_run();
>
> for (i = 0; i < backend_last; i++) {
> - unlink(img_file_name[i]);
> + if (img_file_name[i]) {
> + unlink(img_file_name[i]);
> + }
> }
>
> return ret;
And what terrible, terrible things unlink()'s going to do when passed a
null pointer? Turns out the same scary terrible thing it has always
done: return -1 and set errno = EFAULT.
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [Qemu-devel] [PATCH] tests/hd-geo-test: Don't pass NULL to unlink()
2016-08-05 11:19 ` Markus Armbruster
@ 2016-08-05 11:57 ` Peter Maydell
0 siblings, 0 replies; 9+ messages in thread
From: Peter Maydell @ 2016-08-05 11:57 UTC (permalink / raw)
To: Markus Armbruster; +Cc: QEMU Developers, Patch Tracking
On 5 August 2016 at 12:19, Markus Armbruster <armbru@redhat.com> wrote:
> Peter Maydell <peter.maydell@linaro.org> writes:
>
>> The unlink() function doesn't accept a NULL pointer, so
>> don't pass it one. Spotted by the clang sanitizer.
>>
>> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
>> ---
>> tests/hd-geo-test.c | 4 +++-
>> 1 file changed, 3 insertions(+), 1 deletion(-)
>>
>> diff --git a/tests/hd-geo-test.c b/tests/hd-geo-test.c
>> index 12ee392..6176e81 100644
>> --- a/tests/hd-geo-test.c
>> +++ b/tests/hd-geo-test.c
>> @@ -416,7 +416,9 @@ int main(int argc, char **argv)
>> ret = g_test_run();
>>
>> for (i = 0; i < backend_last; i++) {
>> - unlink(img_file_name[i]);
>> + if (img_file_name[i]) {
>> + unlink(img_file_name[i]);
>> + }
>> }
>>
>> return ret;
>
> And what terrible, terrible things unlink()'s going to do when passed a
> null pointer? Turns out the same scary terrible thing it has always
> done: return -1 and set errno = EFAULT.
Feel free to send a bug report to the glibc folks saying
they shouldn't have marked it as __nonnull((1)).
thanks
-- PMM
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [Qemu-devel] [PATCH] tests/hd-geo-test: Don't pass NULL to unlink()
2016-08-05 10:03 [Qemu-devel] [PATCH] tests/hd-geo-test: Don't pass NULL to unlink() Peter Maydell
2016-08-05 11:19 ` Markus Armbruster
@ 2016-09-06 12:47 ` Peter Maydell
2016-09-06 19:06 ` John Snow
2016-09-08 10:27 ` Peter Maydell
2 siblings, 1 reply; 9+ messages in thread
From: Peter Maydell @ 2016-09-06 12:47 UTC (permalink / raw)
To: QEMU Developers; +Cc: Patch Tracking
Ping?
thanks
-- PMM
On 5 August 2016 at 11:03, Peter Maydell <peter.maydell@linaro.org> wrote:
> The unlink() function doesn't accept a NULL pointer, so
> don't pass it one. Spotted by the clang sanitizer.
>
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
> tests/hd-geo-test.c | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/tests/hd-geo-test.c b/tests/hd-geo-test.c
> index 12ee392..6176e81 100644
> --- a/tests/hd-geo-test.c
> +++ b/tests/hd-geo-test.c
> @@ -416,7 +416,9 @@ int main(int argc, char **argv)
> ret = g_test_run();
>
> for (i = 0; i < backend_last; i++) {
> - unlink(img_file_name[i]);
> + if (img_file_name[i]) {
> + unlink(img_file_name[i]);
> + }
> }
>
> return ret;
> --
> 2.7.4
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [Qemu-devel] [PATCH] tests/hd-geo-test: Don't pass NULL to unlink()
2016-09-06 12:47 ` Peter Maydell
@ 2016-09-06 19:06 ` John Snow
2016-09-06 19:07 ` Peter Maydell
0 siblings, 1 reply; 9+ messages in thread
From: John Snow @ 2016-09-06 19:06 UTC (permalink / raw)
To: Peter Maydell, QEMU Developers; +Cc: Patch Tracking
On 09/06/2016 08:47 AM, Peter Maydell wrote:
> Ping?
>
> thanks
> -- PMM
>
Not sure who this belongs to; I can queue it up alongside some IDE and
FDC stuff in the future if you like.
--js
>
> On 5 August 2016 at 11:03, Peter Maydell <peter.maydell@linaro.org> wrote:
>> The unlink() function doesn't accept a NULL pointer, so
>> don't pass it one. Spotted by the clang sanitizer.
>>
>> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
>> ---
>> tests/hd-geo-test.c | 4 +++-
>> 1 file changed, 3 insertions(+), 1 deletion(-)
>>
>> diff --git a/tests/hd-geo-test.c b/tests/hd-geo-test.c
>> index 12ee392..6176e81 100644
>> --- a/tests/hd-geo-test.c
>> +++ b/tests/hd-geo-test.c
>> @@ -416,7 +416,9 @@ int main(int argc, char **argv)
>> ret = g_test_run();
>>
>> for (i = 0; i < backend_last; i++) {
>> - unlink(img_file_name[i]);
>> + if (img_file_name[i]) {
>> + unlink(img_file_name[i]);
>> + }
>> }
>>
>> return ret;
>> --
>> 2.7.4
>
--
—js
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [Qemu-devel] [PATCH] tests/hd-geo-test: Don't pass NULL to unlink()
2016-09-06 19:06 ` John Snow
@ 2016-09-06 19:07 ` Peter Maydell
2016-09-06 19:15 ` John Snow
0 siblings, 1 reply; 9+ messages in thread
From: Peter Maydell @ 2016-09-06 19:07 UTC (permalink / raw)
To: John Snow; +Cc: QEMU Developers, Patch Tracking
On 6 September 2016 at 20:06, John Snow <jsnow@redhat.com> wrote:
> On 09/06/2016 08:47 AM, Peter Maydell wrote:
>>
>> Ping?
> Not sure who this belongs to; I can queue it up alongside some IDE and FDC
> stuff in the future if you like.
I can apply it directly; I'd just like review :-)
thanks
-- PMM
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [Qemu-devel] [PATCH] tests/hd-geo-test: Don't pass NULL to unlink()
2016-09-06 19:07 ` Peter Maydell
@ 2016-09-06 19:15 ` John Snow
2016-09-06 19:52 ` Peter Maydell
0 siblings, 1 reply; 9+ messages in thread
From: John Snow @ 2016-09-06 19:15 UTC (permalink / raw)
To: Peter Maydell; +Cc: QEMU Developers, Patch Tracking
On 09/06/2016 03:07 PM, Peter Maydell wrote:
> On 6 September 2016 at 20:06, John Snow <jsnow@redhat.com> wrote:
>> On 09/06/2016 08:47 AM, Peter Maydell wrote:
>>>
>>> Ping?
>
>> Not sure who this belongs to; I can queue it up alongside some IDE and FDC
>> stuff in the future if you like.
>
> I can apply it directly; I'd just like review :-)
>
lol! yes, of course you can...
> thanks
> -- PMM
>
Well, I know some have religious arguments against making sanitizers
happy, but that's not my religion.
If some systems appear to think that unlink must take a nonnull
argument, I think that's a bug -- but making the sanitizer happy doesn't
cost us much either IMO.
(And it's /just/ a test.)
So, personally:
Reviewed-by: John Snow <jsnow@redhat.com>
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [Qemu-devel] [PATCH] tests/hd-geo-test: Don't pass NULL to unlink()
2016-09-06 19:15 ` John Snow
@ 2016-09-06 19:52 ` Peter Maydell
0 siblings, 0 replies; 9+ messages in thread
From: Peter Maydell @ 2016-09-06 19:52 UTC (permalink / raw)
To: John Snow; +Cc: QEMU Developers, Patch Tracking
On 6 September 2016 at 20:15, John Snow <jsnow@redhat.com> wrote:
> Well, I know some have religious arguments against making sanitizers happy,
> but that's not my religion.
>
> If some systems appear to think that unlink must take a nonnull argument, I
> think that's a bug -- but making the sanitizer happy doesn't cost us much
> either IMO.
POSIX doesn't mandate that unlink() handles NULL, so it's just
a quality-of-implementation issue in the library to handle it
without barfing (or conversely a QoI issue to over-conservatively
mark it as requires-nonnull in the system headers, if you like).
In any case my general view with all these sanitizing/lint tools
is that it's usually worth taking a few unnecessary-looking
detours in order to get to zero-warnings so you can easily
flag up when new code triggers a warning (which could well
be a bug).
> (And it's /just/ a test.)
>
> So, personally:
>
> Reviewed-by: John Snow <jsnow@redhat.com>
Thanks!
-- PMM
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [Qemu-devel] [PATCH] tests/hd-geo-test: Don't pass NULL to unlink()
2016-08-05 10:03 [Qemu-devel] [PATCH] tests/hd-geo-test: Don't pass NULL to unlink() Peter Maydell
2016-08-05 11:19 ` Markus Armbruster
2016-09-06 12:47 ` Peter Maydell
@ 2016-09-08 10:27 ` Peter Maydell
2 siblings, 0 replies; 9+ messages in thread
From: Peter Maydell @ 2016-09-08 10:27 UTC (permalink / raw)
To: QEMU Developers; +Cc: Patch Tracking
On 5 August 2016 at 11:03, Peter Maydell <peter.maydell@linaro.org> wrote:
> The unlink() function doesn't accept a NULL pointer, so
> don't pass it one. Spotted by the clang sanitizer.
>
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
> tests/hd-geo-test.c | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/tests/hd-geo-test.c b/tests/hd-geo-test.c
> index 12ee392..6176e81 100644
> --- a/tests/hd-geo-test.c
> +++ b/tests/hd-geo-test.c
> @@ -416,7 +416,9 @@ int main(int argc, char **argv)
> ret = g_test_run();
>
> for (i = 0; i < backend_last; i++) {
> - unlink(img_file_name[i]);
> + if (img_file_name[i]) {
> + unlink(img_file_name[i]);
> + }
> }
>
> return ret;
Applied to master, thanks.
-- PMM
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2016-09-08 10:28 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-08-05 10:03 [Qemu-devel] [PATCH] tests/hd-geo-test: Don't pass NULL to unlink() Peter Maydell
2016-08-05 11:19 ` Markus Armbruster
2016-08-05 11:57 ` Peter Maydell
2016-09-06 12:47 ` Peter Maydell
2016-09-06 19:06 ` John Snow
2016-09-06 19:07 ` Peter Maydell
2016-09-06 19:15 ` John Snow
2016-09-06 19:52 ` Peter Maydell
2016-09-08 10:27 ` Peter Maydell
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.