Re: [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 <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.

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