script, broken test options/return

script, broken test options/return

[Ruediger Meier]

Hi,

on my system (interactive bash on openSUSE 13.1) the "return" subtest 
of "script/options" is broken since 54c6611d.

Seems that input from /dev/null makes the difference. Reproduce like
this:

$ export SCRIPT_DEBUG=all 
$ ./script --command "exit 1" out_good >/dev/null            2>dbg_good
$ ./script --command "exit 1" out_bad  >/dev/null </dev/null 2>dbg_bad
^C <abort ctrl-C>

$ head -n 20 dbg_good dbg_bad
==> dbg_good <==
25703: script:       IO: master fd: 3
25703: script:   SIGNAL: signal fd=5
25703: script:     POLL: calling poll()
25703: script:     POLL: poll() rc=1
25703: script:     POLL:  active pfd[signal].fd=5 POLLIN
25703: script:   SIGNAL: signal FD 5 active
25703: script:     MISC: waiting for child
25703: script:     POLL: calling poll()
25703: script:     POLL: poll() rc=0
25703: script:     POLL: setting die=1
25703: script:     POLL: poll() done
25703: script:     MISC: done!

==> dbg_bad <==
25708: script:       IO: master fd: 3
25708: script:   SIGNAL: signal fd=5
25708: script:     POLL: calling poll()
25708: script:     POLL: poll() rc=1
25708: script:     POLL:  active pfd[stdin].fd=0 POLLIN
25708: script:       IO: 0 FD active
25708: script:     POLL: calling poll()
25708: script:     POLL: poll() rc=1
25708: script:     POLL:  active pfd[stdin].fd=0 POLLIN
25708: script:       IO: 0 FD active
25708: script:     POLL: calling poll()
25708: script:     POLL: poll() rc=1
25708: script:     POLL:  active pfd[stdin].fd=0 POLLIN
25708: script:       IO: 0 FD active
25708: script:     POLL: calling poll()
25708: script:     POLL: poll() rc=1
25708: script:     POLL:  active pfd[stdin].fd=0 POLLIN
25708: script:       IO: 0 FD active
25708: script:     POLL: calling poll()
25708: script:     POLL: poll() rc=1
...

cu,
Rudi

Re: script, broken test options/return

[Karel Zak]

On Thu, Jul 02, 2015 at 02:03:21AM +0200, Ruediger Meier wrote:
> on my system (interactive bash on openSUSE 13.1) the "return" subtest 
> of "script/options" is broken since 54c6611d.
> 
> Seems that input from /dev/null makes the difference. Reproduce like
> this:
> 
> $ export SCRIPT_DEBUG=all 
> $ ./script --command "exit 1" out_good >/dev/null            2>dbg_good
> $ ./script --command "exit 1" out_bad  >/dev/null </dev/null 2>dbg_bad
> ^C <abort ctrl-C>

 Does is mean that script(1) hangs?

 Anyway, I'm not able to reproduce the problem, script as well as the
 test(s) work as expected (bash-4.3.39-1).

    Karel
-- 
 Karel Zak  <kzak@redhat.com>
 http://karelzak.blogspot.com

Re: script, broken test options/return

[Karel Zak]

On Thu, Jul 02, 2015 at 02:03:21AM +0200, Ruediger Meier wrote:
> 25708: script:       IO: master fd: 3
> 25708: script:   SIGNAL: signal fd=5
> 25708: script:     POLL: calling poll()
> 25708: script:     POLL: poll() rc=1
> 25708: script:     POLL:  active pfd[stdin].fd=0 POLLIN
> 25708: script:       IO: 0 FD active
> 25708: script:     POLL: calling poll()
> 25708: script:     POLL: poll() rc=1
> 25708: script:     POLL:  active pfd[stdin].fd=0 POLLIN
> 25708: script:       IO: 0 FD active
> 25708: script:     POLL: calling poll()
> 25708: script:     POLL: poll() rc=1
> 25708: script:     POLL:  active pfd[stdin].fd=0 POLLIN
> 25708: script:       IO: 0 FD active
> 25708: script:     POLL: calling poll()
> 25708: script:     POLL: poll() rc=1
> 25708: script:     POLL:  active pfd[stdin].fd=0 POLLIN
> 25708: script:       IO: 0 FD active
> 25708: script:     POLL: calling poll()
> 25708: script:     POLL: poll() rc=1

It seems that handle_io() is not able to detect EOF, I have pushed a
commit that makes sure errno is zero (I guess it's the problem).

Please, try it. Thanks.

    Karel


-- 
 Karel Zak  <kzak@redhat.com>
 http://karelzak.blogspot.com

Re: script, broken test options/return

[Ruediger Meier]

On Thursday 02 July 2015, Karel Zak wrote:

> It seems that handle_io() is not able to detect EOF, I have pushed a
> commit that makes sure errno is zero (I guess it's the problem).
>
> Please, try it. Thanks.

Thanks, this works now. But I'm going to push another patch which is IMO 
a bit cleaner and to address another potential issue.

cu,
Rudi

[PATCH] script: evaluate errno only if read() sets it

[Ruediger Meier]

From: Ruediger Meier <ruediger.meier@ga-group.nl>

Signed-off-by: Ruediger Meier <ruediger.meier@ga-group.nl>
---
 term-utils/script.c | 8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)

diff --git a/term-utils/script.c b/term-utils/script.c
index 9fdef2e..75881a6 100644
--- a/term-utils/script.c
+++ b/term-utils/script.c
@@ -322,7 +322,6 @@ static void handle_io(struct script_control *ctl, int fd, int *eof)
 
 	DBG(IO, ul_debug("%d FD active", fd));
 	*eof = 0;
-	errno = 0;
 
 	/* read from active FD */
 	bytes = read(fd, buf, sizeof(buf));
@@ -334,8 +333,7 @@ static void handle_io(struct script_control *ctl, int fd, int *eof)
 	}
 
 	if (bytes == 0) {
-		if (errno == 0)
-			*eof = 1;
+		*eof = 1;
 		return;
 	}
 
@@ -367,7 +365,7 @@ static void handle_signal(struct script_control *ctl, int fd)
 
 	bytes = read(fd, &info, sizeof(info));
 	if (bytes != sizeof(info)) {
-		if (errno == EAGAIN)
+		if (bytes < 0 && errno == EAGAIN)
 			return;
 		fail(ctl);
 	}
@@ -463,7 +461,7 @@ static void do_io(struct script_control *ctl)
 					handle_io(ctl, pfd[i].fd, &eof);
 				/* EOF maybe detected by two ways:
 				 *	A) poll() return POLLHUP event after close()
-				 *	B) read() returns no error and no data */
+				 *	B) read() returns 0 (no data) */
 				if ((pfd[i].revents & POLLHUP) || eof) {
 					DBG(POLL, ul_debug(" ignore FD"));
 					pfd[i].fd = -1;
-- 
1.8.4.5

Re: [PATCH] script: evaluate errno only if read() sets it

[Ruediger Meier]

I have a question and a comment about my own patch :)

On Thursday 02 July 2015, Ruediger Meier wrote:

> @@ -367,7 +365,7 @@ static void handle_signal(struct script_control
> *ctl, int fd)
>
>  	bytes = read(fd, &info, sizeof(info));
>  	if (bytes != sizeof(info)) {
> -		if (errno == EAGAIN)
> +		if (bytes < 0 && errno == EAGAIN)
>  			return;

Should we also return on EINTR here like we do in handle_io()?

>  		fail(ctl);
>  	}


And I see some more potential problems. For example see this snippet 
from handle_io();

	bytes = read(fd, buf, sizeof(buf));
	if (bytes < 0) {
		DBG(IO, ul_debug(" read failed"));
		if (errno == EAGAIN || errno == EINTR)
			return;
		fail(ctl);
	}

We are using errno after DBG(). But in theory printf() or whatever we do 
in DBG() could change errno. Wouldn't it be a good idea to write 
generally all DBG macros errno-invariant?

cu,
Rudi

Re: [PATCH] script: evaluate errno only if read() sets it

[Karel Zak]

On Thu, Jul 02, 2015 at 12:21:02PM +0200, Ruediger Meier wrote:
> I have a question and a comment about my own patch :)

 :-)

I have applied the patch with some small changes:

> On Thursday 02 July 2015, Ruediger Meier wrote:
> 
> > @@ -367,7 +365,7 @@ static void handle_signal(struct script_control
> > *ctl, int fd)
> >
> >  	bytes = read(fd, &info, sizeof(info));
> >  	if (bytes != sizeof(info)) {
> > -		if (errno == EAGAIN)
> > +		if (bytes < 0 && errno == EAGAIN)
> >  			return;
> 
> Should we also return on EINTR here like we do in handle_io()?

It's probably more robust, Fixed.

> And I see some more potential problems. For example see this snippet 
> from handle_io();
> 
> 	bytes = read(fd, buf, sizeof(buf));
> 	if (bytes < 0) {
> 		DBG(IO, ul_debug(" read failed"));
> 		if (errno == EAGAIN || errno == EINTR)
> 			return;
> 		fail(ctl);
> 	}
> 
> We are using errno after DBG(). But in theory printf() or whatever we do 
> in DBG() could change errno. Wouldn't it be a good idea to write 
> generally all DBG macros errno-invariant?

It seems like overkill solution to modify DBG() and always save errno
on all places. I think it's better to care about this locally on place
where you use DBG(). Fixed in script(1).

    Karel

-- 
 Karel Zak  <kzak@redhat.com>
 http://karelzak.blogspot.com

Re: [PATCH] script: evaluate errno only if read() sets it

[Ruediger Meier]

On Friday 03 July 2015, Karel Zak wrote:
> On Thu, Jul 02, 2015 at 12:21:02PM +0200, Ruediger Meier wrote:
> > I have a question and a comment about my own patch :)
> >
>  :-)
>
> I have applied the patch with some small changes:
> > On Thursday 02 July 2015, Ruediger Meier wrote:
> > > @@ -367,7 +365,7 @@ static void handle_signal(struct
> > > script_control *ctl, int fd)
> > >
> > >  	bytes = read(fd, &info, sizeof(info));
> > >  	if (bytes != sizeof(info)) {
> > > -		if (errno == EAGAIN)
> > > +		if (bytes < 0 && errno == EAGAIN)
> > >  			return;
> >
> > Should we also return on EINTR here like we do in handle_io()?
>
> It's probably more robust, Fixed.

BTW there is another potential issue for systems where EAGAIN != 
EWOULDBLOCK. These systems might be seldom but who knows exactly? If we 
don't want to care about EWOULDBLOCK in whole util-linux then we 
should maybe abort configure if both are not equal ... and wait for 
complaining users.

see
http://stackoverflow.com/questions/7003234/which-systems-define-eagain-and-ewouldblock-as-different-values

I've tested on AIX but when using gcc and/or the right defines I've
got EAGAIN == EWOULDBLOCK there too.

cu,
Rudi

[PATCH] tests: mkfs.ext3 image-file needs option -F

[Ruediger Meier]

From: Ruediger Meier <ruediger.meier@ga-group.nl>

Maybe on newer systems it's not needed anymore.

Signed-off-by: Ruediger Meier <ruediger.meier@ga-group.nl>
---
 tests/ts/mount/fstab-loop | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tests/ts/mount/fstab-loop b/tests/ts/mount/fstab-loop
index 3178b95..9e17943 100755
--- a/tests/ts/mount/fstab-loop
+++ b/tests/ts/mount/fstab-loop
@@ -30,7 +30,7 @@ ts_check_prog "mkfs.ext3"
 
 IMG=$(ts_image_init)
 
-mkfs.ext3 $IMG &> /dev/null || ts_die "Cannot make ext3 on $DEVICE"
+mkfs.ext3 -F $IMG &> /dev/null || ts_die "Cannot make ext3 on $IMG"
 
 [ -d "$TS_MOUNTPOINT-1" ] || mkdir -p $TS_MOUNTPOINT-1
 [ -d "$TS_MOUNTPOINT-2" ] || mkdir -p $TS_MOUNTPOINT-2
-- 
1.8.4.5

Re: [PATCH] tests: mkfs.ext3 image-file needs option -F

[Ruediger Meier]

Oops, by accident I've used the wrong --in-reply-to from my shell 
history.

On Friday 12 February 2016, Ruediger Meier wrote:
> From: Ruediger Meier <ruediger.meier@ga-group.nl>
>
> Maybe on newer systems it's not needed anymore.
>
> Signed-off-by: Ruediger Meier <ruediger.meier@ga-group.nl>
> ---
>  tests/ts/mount/fstab-loop | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/tests/ts/mount/fstab-loop b/tests/ts/mount/fstab-loop
> index 3178b95..9e17943 100755
> --- a/tests/ts/mount/fstab-loop
> +++ b/tests/ts/mount/fstab-loop
> @@ -30,7 +30,7 @@ ts_check_prog "mkfs.ext3"
>
>  IMG=$(ts_image_init)
>
> -mkfs.ext3 $IMG &> /dev/null || ts_die "Cannot make ext3 on $DEVICE"
> +mkfs.ext3 -F $IMG &> /dev/null || ts_die "Cannot make ext3 on $IMG"
>
>  [ -d "$TS_MOUNTPOINT-1" ] || mkdir -p $TS_MOUNTPOINT-1
>  [ -d "$TS_MOUNTPOINT-2" ] || mkdir -p $TS_MOUNTPOINT-2