[Qemu-devel] [PATCH v7] tests/bios-tables-test: check the value returned by fopen()

[Qemu-devel] [PATCH v7] tests/bios-tables-test: check the value returned by fopen()

[zhanghailiang]

The function fopen() may fail, so check its return value.

Signed-off-by: zhanghailiang <zhang.zhanghailiang@huawei.com>
Signed-off-by: Li Liu <john.liuli@huawei.com>
Reviewed-by: Alex Bennée <alex.bennee@linaro.org>
---
 tests/bios-tables-test.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/tests/bios-tables-test.c b/tests/bios-tables-test.c
index 045eb27..feb3b58 100644
--- a/tests/bios-tables-test.c
+++ b/tests/bios-tables-test.c
@@ -790,6 +790,11 @@ int main(int argc, char *argv[])
     const char *arch = qtest_get_arch();
     FILE *f = fopen(disk, "w");
     int ret;
+
+    if (!f) {
+        fprintf(stderr, "Couldn't open \"%s\": %s", disk, strerror(errno));
+        return -1;
+    }
     fwrite(boot_sector, 1, sizeof boot_sector, f);
     fclose(f);
 
-- 
1.7.12.4

Re: [Qemu-devel] [PATCH v7] tests/bios-tables-test: check the value returned by fopen()

[Peter Maydell]

On 18 August 2014 08:54, zhanghailiang <zhang.zhanghailiang@huawei.com> wrote:
> The function fopen() may fail, so check its return value.
>
> Signed-off-by: zhanghailiang <zhang.zhanghailiang@huawei.com>
> Signed-off-by: Li Liu <john.liuli@huawei.com>
> Reviewed-by: Alex Bennée <alex.bennee@linaro.org>
> ---
>  tests/bios-tables-test.c | 5 +++++
>  1 file changed, 5 insertions(+)
>
> diff --git a/tests/bios-tables-test.c b/tests/bios-tables-test.c
> index 045eb27..feb3b58 100644
> --- a/tests/bios-tables-test.c
> +++ b/tests/bios-tables-test.c
> @@ -790,6 +790,11 @@ int main(int argc, char *argv[])
>      const char *arch = qtest_get_arch();
>      FILE *f = fopen(disk, "w");
>      int ret;
> +
> +    if (!f) {
> +        fprintf(stderr, "Couldn't open \"%s\": %s", disk, strerror(errno));
> +        return -1;
> +    }

-1 isn't a sensible value to return from main(); you want 1.
(Program exit statuses can only be 0 or positive.)

thanks
-- PMM

Re: [Qemu-devel] [PATCH v7] tests/bios-tables-test: check the value returned by fopen()

[Marcel Apfelbaum]

On Mon, 2014-08-18 at 15:54 +0800, zhanghailiang wrote:
> The function fopen() may fail, so check its return value.
> 
> Signed-off-by: zhanghailiang <zhang.zhanghailiang@huawei.com>
> Signed-off-by: Li Liu <john.liuli@huawei.com>
> Reviewed-by: Alex Bennée <alex.bennee@linaro.org>
> ---
>  tests/bios-tables-test.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/tests/bios-tables-test.c b/tests/bios-tables-test.c
> index 045eb27..feb3b58 100644
> --- a/tests/bios-tables-test.c
> +++ b/tests/bios-tables-test.c
> @@ -790,6 +790,11 @@ int main(int argc, char *argv[])
>      const char *arch = qtest_get_arch();
>      FILE *f = fopen(disk, "w");
>      int ret;
> +
> +    if (!f) {
> +        fprintf(stderr, "Couldn't open \"%s\": %s", disk, strerror(errno));
> +        return -1;
> +    }
Hi,
I think we should use an assert here, we need an indication
that the test failed and a print to stderr is not enough.

Thanks,
Marcel
>      fwrite(boot_sector, 1, sizeof boot_sector, f);
>      fclose(f);
>  

Re: [Qemu-devel] [Qemu-trivial] [PATCH v7] tests/bios-tables-test: check the value returned by fopen()

[Michael Tokarev]

18.08.2014 13:13, Marcel Apfelbaum wrote:
> On Mon, 2014-08-18 at 15:54 +0800, zhanghailiang wrote:
>> The function fopen() may fail, so check its return value.
>>
>> Signed-off-by: zhanghailiang <zhang.zhanghailiang@huawei.com>
>> Signed-off-by: Li Liu <john.liuli@huawei.com>
>> Reviewed-by: Alex Bennée <alex.bennee@linaro.org>
>> ---
>>  tests/bios-tables-test.c | 5 +++++
>>  1 file changed, 5 insertions(+)
>>
>> diff --git a/tests/bios-tables-test.c b/tests/bios-tables-test.c
>> index 045eb27..feb3b58 100644
>> --- a/tests/bios-tables-test.c
>> +++ b/tests/bios-tables-test.c
>> @@ -790,6 +790,11 @@ int main(int argc, char *argv[])
>>      const char *arch = qtest_get_arch();
>>      FILE *f = fopen(disk, "w");
>>      int ret;
>> +
>> +    if (!f) {
>> +        fprintf(stderr, "Couldn't open \"%s\": %s", disk, strerror(errno));
>> +        return -1;
>> +    }
> Hi,
> I think we should use an assert here, we need an indication
> that the test failed and a print to stderr is not enough.

Guys, please stop misusing (or trying to misuse) assert().  assert() is for
code path which are impossible by program logic.  Here, it is a error check,
not a code logic check -- the fopen() _may_ fail, and this is not something
the code around makes impossible.  So in cases like this (and in other similar
case like vvfat.log open check), we should either print to stderr and exit,
or print elsewhere, but we should NOT use assert().  Think what will be if
the program will be compiled with -D_NDEBUG and all assert()s are turned into
a no-op.

So, no assert() in cases like this here and elsewhere (not only in qemu). It
is not what assert() is provided for.

Thanks,

/mjt

Re: [Qemu-devel] [Qemu-trivial] [PATCH v7] tests/bios-tables-test: check the value returned by fopen()

[Michael Tokarev]

18.08.2014 11:54, zhanghailiang wrote:
> The function fopen() may fail, so check its return value.
> 
> Signed-off-by: zhanghailiang <zhang.zhanghailiang@huawei.com>
> Signed-off-by: Li Liu <john.liuli@huawei.com>
> Reviewed-by: Alex Bennée <alex.bennee@linaro.org>
> ---
>  tests/bios-tables-test.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/tests/bios-tables-test.c b/tests/bios-tables-test.c
> index 045eb27..feb3b58 100644
> --- a/tests/bios-tables-test.c
> +++ b/tests/bios-tables-test.c
> @@ -790,6 +790,11 @@ int main(int argc, char *argv[])
>      const char *arch = qtest_get_arch();
>      FILE *f = fopen(disk, "w");
>      int ret;
> +
> +    if (!f) {
> +        fprintf(stderr, "Couldn't open \"%s\": %s", disk, strerror(errno));
> +        return -1;
> +    }

Applied to -trivial, with s/-1/1/.  No need to resend it again.

Thanks,

/mjt

>      fwrite(boot_sector, 1, sizeof boot_sector, f);
>      fclose(f);
>  
> 

Re: [Qemu-devel] [Qemu-trivial] [PATCH v7] tests/bios-tables-test: check the value returned by fopen()

[Marcel Apfelbaum]

On Mon, 2014-08-18 at 15:37 +0400, Michael Tokarev wrote:
> 18.08.2014 13:13, Marcel Apfelbaum wrote:
> > On Mon, 2014-08-18 at 15:54 +0800, zhanghailiang wrote:
> >> The function fopen() may fail, so check its return value.
> >>
> >> Signed-off-by: zhanghailiang <zhang.zhanghailiang@huawei.com>
> >> Signed-off-by: Li Liu <john.liuli@huawei.com>
> >> Reviewed-by: Alex Bennée <alex.bennee@linaro.org>
> >> ---
> >>  tests/bios-tables-test.c | 5 +++++
> >>  1 file changed, 5 insertions(+)
> >>
> >> diff --git a/tests/bios-tables-test.c b/tests/bios-tables-test.c
> >> index 045eb27..feb3b58 100644
> >> --- a/tests/bios-tables-test.c
> >> +++ b/tests/bios-tables-test.c
> >> @@ -790,6 +790,11 @@ int main(int argc, char *argv[])
> >>      const char *arch = qtest_get_arch();
> >>      FILE *f = fopen(disk, "w");
> >>      int ret;
> >> +
> >> +    if (!f) {
> >> +        fprintf(stderr, "Couldn't open \"%s\": %s", disk, strerror(errno));
> >> +        return -1;
> >> +    }
> > Hi,
> > I think we should use an assert here, we need an indication
> > that the test failed and a print to stderr is not enough.
> 
> Guys, please stop misusing (or trying to misuse) assert().  assert() is for
> code path which are impossible by program logic.  Here, it is a error check,
> not a code logic check -- the fopen() _may_ fail, and this is not something
> the code around makes impossible.  So in cases like this (and in other similar
> case like vvfat.log open check), we should either print to stderr and exit,
> or print elsewhere, but we should NOT use assert().  Think what will be if
> the program will be compiled with -D_NDEBUG and all assert()s are turned into
> a no-op.
> 
> So, no assert() in cases like this here and elsewhere (not only in qemu). It
> is not what assert() is provided for.

Hi Michael,
Thanks for the reminder, it really helps.

While I agree 100% with your explanation, I was thinking to use
assert only because there would be no reason for the fopen to fail
while creating a file in current directory.
But fopen *may* fail(no writing rights for current directory) and this is an error check.

Thanks again,
Marcel

> 
> Thanks,
> 
> /mjt

Re: [Qemu-devel] [Qemu-trivial] [PATCH v7] tests/bios-tables-test: check the value returned by fopen()

[Michael S. Tsirkin]

On Mon, Aug 18, 2014 at 03:38:12PM +0400, Michael Tokarev wrote:
> 18.08.2014 11:54, zhanghailiang wrote:
> > The function fopen() may fail, so check its return value.
> > 
> > Signed-off-by: zhanghailiang <zhang.zhanghailiang@huawei.com>
> > Signed-off-by: Li Liu <john.liuli@huawei.com>
> > Reviewed-by: Alex Bennée <alex.bennee@linaro.org>
> > ---
> >  tests/bios-tables-test.c | 5 +++++
> >  1 file changed, 5 insertions(+)
> > 
> > diff --git a/tests/bios-tables-test.c b/tests/bios-tables-test.c
> > index 045eb27..feb3b58 100644
> > --- a/tests/bios-tables-test.c
> > +++ b/tests/bios-tables-test.c
> > @@ -790,6 +790,11 @@ int main(int argc, char *argv[])
> >      const char *arch = qtest_get_arch();
> >      FILE *f = fopen(disk, "w");
> >      int ret;
> > +
> > +    if (!f) {
> > +        fprintf(stderr, "Couldn't open \"%s\": %s", disk, strerror(errno));
> > +        return -1;
> > +    }
> 
> Applied to -trivial, with s/-1/1/.  No need to resend it again.
> 
> Thanks,
> 
> /mjt
> 
> >      fwrite(boot_sector, 1, sizeof boot_sector, f);
> >      fclose(f);
> >  
> > 

This doesn't seem appropriate to trivial tree:
there were some objections from Marcel, I'd like to
see them addressed.

Re: [Qemu-devel] [Qemu-trivial] [PATCH v7] tests/bios-tables-test: check the value returned by fopen()

[Michael S. Tsirkin]

On Mon, Aug 18, 2014 at 03:37:16PM +0400, Michael Tokarev wrote:
> 18.08.2014 13:13, Marcel Apfelbaum wrote:
> > On Mon, 2014-08-18 at 15:54 +0800, zhanghailiang wrote:
> >> The function fopen() may fail, so check its return value.
> >>
> >> Signed-off-by: zhanghailiang <zhang.zhanghailiang@huawei.com>
> >> Signed-off-by: Li Liu <john.liuli@huawei.com>
> >> Reviewed-by: Alex Bennée <alex.bennee@linaro.org>
> >> ---
> >>  tests/bios-tables-test.c | 5 +++++
> >>  1 file changed, 5 insertions(+)
> >>
> >> diff --git a/tests/bios-tables-test.c b/tests/bios-tables-test.c
> >> index 045eb27..feb3b58 100644
> >> --- a/tests/bios-tables-test.c
> >> +++ b/tests/bios-tables-test.c
> >> @@ -790,6 +790,11 @@ int main(int argc, char *argv[])
> >>      const char *arch = qtest_get_arch();
> >>      FILE *f = fopen(disk, "w");
> >>      int ret;
> >> +
> >> +    if (!f) {
> >> +        fprintf(stderr, "Couldn't open \"%s\": %s", disk, strerror(errno));
> >> +        return -1;
> >> +    }
> > Hi,
> > I think we should use an assert here, we need an indication
> > that the test failed and a print to stderr is not enough.
> 
> Guys, please stop misusing (or trying to misuse) assert().  assert() is for
> code path which are impossible by program logic.  Here, it is a error check,
> not a code logic check -- the fopen() _may_ fail, and this is not something
> the code around makes impossible.  So in cases like this (and in other similar
> case like vvfat.log open check), we should either print to stderr and exit,
> or print elsewhere, but we should NOT use assert().  Think what will be if
> the program will be compiled with -D_NDEBUG and all assert()s are turned into
> a no-op.

We don't support that configuration, a bunch of stuff will be
broken if you try.


> So, no assert() in cases like this here and elsewhere (not only in qemu). It
> is not what assert() is provided for.
> 
> Thanks,
> 
> /mjt

I partially agree in that asserts produce errors which aren't
easy for users to understand, and gives a coredump which
takes up disk space. So if we expect some error to trigger,
we should print a proper error, and avoid producing a coredump.

On the other hand, there are situations where we
take a shortcut and decide that error is highly unlikely
(even if not 100% impossible), so not worth handling.
In these cases assert will make debugging easier
in case it does trigger after all.

This case here is about a test, so it's used
by developers, not users. Thus assert might be appropriate.

-- 
MST

Re: [Qemu-devel] [Qemu-trivial] [PATCH v7] tests/bios-tables-test: check the value returned by fopen()

[Michael Tokarev]

18.08.2014 17:44, Michael S. Tsirkin wrote:
> On Mon, Aug 18, 2014 at 03:38:12PM +0400, Michael Tokarev wrote:
>> 18.08.2014 11:54, zhanghailiang wrote:
>>> The function fopen() may fail, so check its return value.
>>>
>>> Signed-off-by: zhanghailiang <zhang.zhanghailiang@huawei.com>
>>> Signed-off-by: Li Liu <john.liuli@huawei.com>
>>> Reviewed-by: Alex Bennée <alex.bennee@linaro.org>
>>> ---
>>>  tests/bios-tables-test.c | 5 +++++
>>>  1 file changed, 5 insertions(+)
>>>
>>> diff --git a/tests/bios-tables-test.c b/tests/bios-tables-test.c
>>> index 045eb27..feb3b58 100644
>>> --- a/tests/bios-tables-test.c
>>> +++ b/tests/bios-tables-test.c
>>> @@ -790,6 +790,11 @@ int main(int argc, char *argv[])
>>>      const char *arch = qtest_get_arch();
>>>      FILE *f = fopen(disk, "w");
>>>      int ret;
>>> +
>>> +    if (!f) {
>>> +        fprintf(stderr, "Couldn't open \"%s\": %s", disk, strerror(errno));
>>> +        return -1;
>>> +    }
>>
>> Applied to -trivial, with s/-1/1/.  No need to resend it again.
>>
>> Thanks,
>>
>> /mjt
>>
>>>      fwrite(boot_sector, 1, sizeof boot_sector, f);
>>>      fclose(f);
> 
> This doesn't seem appropriate to trivial tree:
> there were some objections from Marcel, I'd like to
> see them addressed.

I see the objections as addressed, don't you?

/mjt

Re: [Qemu-devel] [Qemu-trivial] [PATCH v7] tests/bios-tables-test: check the value returned by fopen()

[Michael S. Tsirkin]

On Mon, Aug 18, 2014 at 06:24:02PM +0400, Michael Tokarev wrote:
> 18.08.2014 17:44, Michael S. Tsirkin wrote:
> > On Mon, Aug 18, 2014 at 03:38:12PM +0400, Michael Tokarev wrote:
> >> 18.08.2014 11:54, zhanghailiang wrote:
> >>> The function fopen() may fail, so check its return value.
> >>>
> >>> Signed-off-by: zhanghailiang <zhang.zhanghailiang@huawei.com>
> >>> Signed-off-by: Li Liu <john.liuli@huawei.com>
> >>> Reviewed-by: Alex Bennée <alex.bennee@linaro.org>
> >>> ---
> >>>  tests/bios-tables-test.c | 5 +++++
> >>>  1 file changed, 5 insertions(+)
> >>>
> >>> diff --git a/tests/bios-tables-test.c b/tests/bios-tables-test.c
> >>> index 045eb27..feb3b58 100644
> >>> --- a/tests/bios-tables-test.c
> >>> +++ b/tests/bios-tables-test.c
> >>> @@ -790,6 +790,11 @@ int main(int argc, char *argv[])
> >>>      const char *arch = qtest_get_arch();
> >>>      FILE *f = fopen(disk, "w");
> >>>      int ret;
> >>> +
> >>> +    if (!f) {
> >>> +        fprintf(stderr, "Couldn't open \"%s\": %s", disk, strerror(errno));
> >>> +        return -1;
> >>> +    }
> >>
> >> Applied to -trivial, with s/-1/1/.  No need to resend it again.
> >>
> >> Thanks,
> >>
> >> /mjt
> >>
> >>>      fwrite(boot_sector, 1, sizeof boot_sector, f);
> >>>      fclose(f);
> > 
> > This doesn't seem appropriate to trivial tree:
> > there were some objections from Marcel, I'd like to
> > see them addressed.
> 
> I see the objections as addressed, don't you?
> 
> /mjt

Does test fail if this path is triggered?

Re: [Qemu-devel] [Qemu-trivial] [PATCH v7] tests/bios-tables-test: check the value returned by fopen()

[Peter Maydell]

On 18 August 2014 20:36, Michael S. Tsirkin <mst@redhat.com> wrote:
> Does test fail if this path is triggered?

If our test harness doesn't report failure when a test
binary returns with a non-zero exit code then the
harness is broken, because there are other test
binaries that rely on that.

thanks
-- PMM