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.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.
>
> 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
