Re: [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 <address@hidden>
> >> Signed-off-by: Li Liu <address@hidden>
> >> Reviewed-by: Alex Bennée <address@hidden>
> >> ---
> >>  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