| Message ID | 1408348473-10104-1-git-send-email-zhang.zhanghailiang@huawei.com |
|---|---|
| State | New |
| Headers | show |
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
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); >
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
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); > >
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
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.
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.
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
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?
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
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);