| Message ID | 20201211094416.26616-1-bogdan.lezhepekov@suse.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [v1] openposix/fork/7-1.c: A bug fix | expand |
Hi! > The function output interferes with the variable errno, that leads to > the false positive result on limited test setups. The issue fixed. > > Signed-off-by: Bogdan Lezhepekov <bogdan.lezhepekov@suse.com> > --- > .../open_posix_testsuite/conformance/interfaces/fork/7-1.c | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > > diff --git a/testcases/open_posix_testsuite/conformance/interfaces/fork/7-1.c b/testcases/open_posix_testsuite/conformance/interfaces/fork/7-1.c > index c3db90c00..4249d713d 100644 > --- a/testcases/open_posix_testsuite/conformance/interfaces/fork/7-1.c > +++ b/testcases/open_posix_testsuite/conformance/interfaces/fork/7-1.c > @@ -53,12 +53,14 @@ static void read_catalog(nl_catd cat, char *who) > { > char *msg = NULL; > int i, j; > - errno = 0; > > #if VERBOSE > 0 > output("Reading the message catalog from %s...\n", who); > #endif > > + /* the output function interferes with errno */ > + errno = 0; > + > for (i = 1; i <= 2; i++) { > for (j = 1; j <= 2; j++) { This is obviously correct, but I would avoid adding the comment, it's kind of obvious that anything that calls to libc may and will interfere with errno. Also the first line of the commit description could be a bit more description, half of the commits pushed to LTP are bugfixes. So maybe something as: openposix/fork/7-1.c: Clear errno correctly ... I can push the patch with these changes if it's okay with you.
Hi, By adding this comment I wanted to stress that the place of "errno = 0" is not randomly chosen, to prevent somebody from moving it back to the beginning of the file :) But if you find it not necessary, then please go ahead. Thank you, Bogdan On Dec 11 2020, at 11:54 am, Cyril Hrubis <chrubis@suse.cz> wrote: > Hi! > > The function output interferes with the variable errno, that leads to > > the false positive result on limited test setups. The issue fixed. > > > > Signed-off-by: Bogdan Lezhepekov <bogdan.lezhepekov@suse.com> > > --- > > .../open_posix_testsuite/conformance/interfaces/fork/7-1.c | 4 +++- > > 1 file changed, 3 insertions(+), 1 deletion(-) > > > > diff --git a/testcases/open_posix_testsuite/conformance/interfaces/fork/7-1.c b/testcases/open_posix_testsuite/conformance/interfaces/fork/7-1.c > > index c3db90c00..4249d713d 100644 > > --- a/testcases/open_posix_testsuite/conformance/interfaces/fork/7-1.c > > +++ b/testcases/open_posix_testsuite/conformance/interfaces/fork/7-1.c > > @@ -53,12 +53,14 @@ static void read_catalog(nl_catd cat, char *who) > > { > > char *msg = NULL; > > int i, j; > > - errno = 0; > > > > #if VERBOSE > 0 > > output("Reading the message catalog from %s...\n", who); > > #endif > > > > + /* the output function interferes with errno */ > > + errno = 0; > > + > > for (i = 1; i <= 2; i++) { > > for (j = 1; j <= 2; j++) { > > This is obviously correct, but I would avoid adding the comment, it's > kind of obvious that anything that calls to libc may and will interfere > with errno. > > Also the first line of the commit description could be a bit more > description, half of the commits pushed to LTP are bugfixes. So maybe > something as: > > openposix/fork/7-1.c: Clear errno correctly > ... > I can push the patch with these changes if it's okay with you. > > -- > Cyril Hrubis > chrubis@suse.cz >
Hi, As follow up after chat with Petr Vorel. A bug in the test was caused by the line "now = localtime(&nw);" in "output" function, as it sets "errno" variable somewhere. -Bogdan On Dec 11 2020, at 12:04 pm, Bogdan Lezhepekov <bogdan.lezhepekov@suse.com> wrote: > Hi, > > By adding this comment I wanted to stress that the place of "errno = 0" is not randomly chosen, to prevent somebody from moving it back to the beginning of the file :) > But if you find it not necessary, then please go ahead. > Thank you, > Bogdan > > On Dec 11 2020, at 11:54 am, Cyril Hrubis <chrubis@suse.cz> wrote: > > Hi! > > > The function output interferes with the variable errno, that leads to > > > the false positive result on limited test setups. The issue fixed. > > > > > > Signed-off-by: Bogdan Lezhepekov <bogdan.lezhepekov@suse.com> > > > --- > > > .../open_posix_testsuite/conformance/interfaces/fork/7-1.c | 4 +++- > > > 1 file changed, 3 insertions(+), 1 deletion(-) > > > > > > diff --git a/testcases/open_posix_testsuite/conformance/interfaces/fork/7-1.c b/testcases/open_posix_testsuite/conformance/interfaces/fork/7-1.c > > > index c3db90c00..4249d713d 100644 > > > --- a/testcases/open_posix_testsuite/conformance/interfaces/fork/7-1.c > > > +++ b/testcases/open_posix_testsuite/conformance/interfaces/fork/7-1.c > > > @@ -53,12 +53,14 @@ static void read_catalog(nl_catd cat, char *who) > > > { > > > char *msg = NULL; > > > int i, j; > > > - errno = 0; > > > > > > #if VERBOSE > 0 > > > output("Reading the message catalog from %s...\n", who); > > > #endif > > > > > > + /* the output function interferes with errno */ > > > + errno = 0; > > > + > > > for (i = 1; i <= 2; i++) { > > > for (j = 1; j <= 2; j++) { > > > > This is obviously correct, but I would avoid adding the comment, it's > > kind of obvious that anything that calls to libc may and will interfere > > with errno. > > > > Also the first line of the commit description could be a bit more > > description, half of the commits pushed to LTP are bugfixes. So maybe > > something as: > > > > openposix/fork/7-1.c: Clear errno correctly > > ... > > I can push the patch with these changes if it's okay with you. > > > > -- > > Cyril Hrubis > > chrubis@suse.cz > > > > > -- > Mailing list info: https://lists.linux.it/listinfo/ltp
Hi! > By adding this comment I wanted to stress that the place of "errno = > 0" is not randomly chosen, to prevent somebody from moving it back to > the beginning of the file :) But if you find it not necessary, then > please go ahead. I think that this still falls under "do not comment the obvious" rule. We do have more than hundred of testcases that do "errno = 0" and as far as I can tell none of them includes a comment that says that we cannot move the code around and I do not think that we should add hundred of comments into these testcases either.
Hi! > As follow up after chat with Petr Vorel. > A bug in the test was caused by the line "now = localtime(&nw);" in "output" function, as it sets "errno" variable somewhere. localtime() shouldn't fail with anything but EOVERFLOW it's a bit strange to see it fail there.
Hi! I've modified the patch as described and pushed, thanks.
diff --git a/testcases/open_posix_testsuite/conformance/interfaces/fork/7-1.c b/testcases/open_posix_testsuite/conformance/interfaces/fork/7-1.c index c3db90c00..4249d713d 100644 --- a/testcases/open_posix_testsuite/conformance/interfaces/fork/7-1.c +++ b/testcases/open_posix_testsuite/conformance/interfaces/fork/7-1.c @@ -53,12 +53,14 @@ static void read_catalog(nl_catd cat, char *who) { char *msg = NULL; int i, j; - errno = 0; #if VERBOSE > 0 output("Reading the message catalog from %s...\n", who); #endif + /* the output function interferes with errno */ + errno = 0; + for (i = 1; i <= 2; i++) { for (j = 1; j <= 2; j++) {
The function output interferes with the variable errno, that leads to the false positive result on limited test setups. The issue fixed. Signed-off-by: Bogdan Lezhepekov <bogdan.lezhepekov@suse.com> --- .../open_posix_testsuite/conformance/interfaces/fork/7-1.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)