[LTP] [PATCH v1] openposix/fork/7-1.c: A bug fix

[LTP] [PATCH v1] openposix/fork/7-1.c: A bug fix

[Bogdan Lezhepekov]

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++) {
 
-- 
2.26.2

[LTP] [PATCH v1] openposix/fork/7-1.c: A bug fix

[Cyril Hrubis]

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

[LTP] [PATCH v1] openposix/fork/7-1.c: A bug fix

[Bogdan Lezhepekov]

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
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.linux.it/pipermail/ltp/attachments/20201211/ac9a9bb0/attachment.htm>

[LTP] [PATCH v1] openposix/fork/7-1.c: A bug fix

[Bogdan Lezhepekov]

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

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.linux.it/pipermail/ltp/attachments/20201211/a140569e/attachment.htm>

[LTP] [PATCH v1] openposix/fork/7-1.c: A bug fix

[Cyril Hrubis]

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.

-- 
Cyril Hrubis
chrubis@suse.cz

[LTP] [PATCH v1] openposix/fork/7-1.c: A bug fix

[Cyril Hrubis]

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.

-- 
Cyril Hrubis
chrubis@suse.cz

[LTP] [PATCH v1] openposix/fork/7-1.c: A bug fix

[Cyril Hrubis]

Hi!
I've modified the patch as described and pushed, thanks.

-- 
Cyril Hrubis
chrubis@suse.cz

[LTP] [PATCH v1] openposix/fork/7-1.c: A bug fix

[Bogdan Lezhepekov]

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++) {
 
-- 
2.26.2