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.zhanghaili...@huawei.com>
> >> Signed-off-by: Li Liu <john.li...@huawei.com>
> >> Reviewed-by: Alex Bennée <alex.ben...@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
