* script, broken test options/return @ 2015-07-02 0:03 Ruediger Meier 2015-07-02 8:23 ` Karel Zak 2015-07-02 8:32 ` Karel Zak 0 siblings, 2 replies; 10+ messages in thread From: Ruediger Meier @ 2015-07-02 0:03 UTC (permalink / raw) To: util-linux 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: script, broken test options/return 2015-07-02 0:03 script, broken test options/return Ruediger Meier @ 2015-07-02 8:23 ` Karel Zak 2015-07-02 8:32 ` Karel Zak 1 sibling, 0 replies; 10+ messages in thread From: Karel Zak @ 2015-07-02 8:23 UTC (permalink / raw) To: Ruediger Meier; +Cc: util-linux 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: script, broken test options/return 2015-07-02 0:03 script, broken test options/return Ruediger Meier 2015-07-02 8:23 ` Karel Zak @ 2015-07-02 8:32 ` Karel Zak 2015-07-02 10:06 ` Ruediger Meier 1 sibling, 1 reply; 10+ messages in thread From: Karel Zak @ 2015-07-02 8:32 UTC (permalink / raw) To: Ruediger Meier; +Cc: util-linux 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: script, broken test options/return 2015-07-02 8:32 ` Karel Zak @ 2015-07-02 10:06 ` Ruediger Meier 2015-07-02 10:10 ` [PATCH] script: evaluate errno only if read() sets it Ruediger Meier 2016-02-12 16:23 ` [PATCH] tests: mkfs.ext3 image-file needs option -F Ruediger Meier 0 siblings, 2 replies; 10+ messages in thread From: Ruediger Meier @ 2015-07-02 10:06 UTC (permalink / raw) To: Karel Zak; +Cc: util-linux 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH] script: evaluate errno only if read() sets it 2015-07-02 10:06 ` Ruediger Meier @ 2015-07-02 10:10 ` Ruediger Meier 2015-07-02 10:21 ` Ruediger Meier 2016-02-12 16:23 ` [PATCH] tests: mkfs.ext3 image-file needs option -F Ruediger Meier 1 sibling, 1 reply; 10+ messages in thread From: Ruediger Meier @ 2015-07-02 10:10 UTC (permalink / raw) To: util-linux 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 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH] script: evaluate errno only if read() sets it 2015-07-02 10:10 ` [PATCH] script: evaluate errno only if read() sets it Ruediger Meier @ 2015-07-02 10:21 ` Ruediger Meier 2015-07-03 8:04 ` Karel Zak 0 siblings, 1 reply; 10+ messages in thread From: Ruediger Meier @ 2015-07-02 10:21 UTC (permalink / raw) To: util-linux 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] script: evaluate errno only if read() sets it 2015-07-02 10:21 ` Ruediger Meier @ 2015-07-03 8:04 ` Karel Zak 2015-07-03 10:33 ` Ruediger Meier 0 siblings, 1 reply; 10+ messages in thread From: Karel Zak @ 2015-07-03 8:04 UTC (permalink / raw) To: Ruediger Meier; +Cc: util-linux 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] script: evaluate errno only if read() sets it 2015-07-03 8:04 ` Karel Zak @ 2015-07-03 10:33 ` Ruediger Meier 0 siblings, 0 replies; 10+ messages in thread From: Ruediger Meier @ 2015-07-03 10:33 UTC (permalink / raw) To: Karel Zak; +Cc: util-linux 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH] tests: mkfs.ext3 image-file needs option -F 2015-07-02 10:06 ` Ruediger Meier 2015-07-02 10:10 ` [PATCH] script: evaluate errno only if read() sets it Ruediger Meier @ 2016-02-12 16:23 ` Ruediger Meier 2016-02-12 16:25 ` Ruediger Meier 1 sibling, 1 reply; 10+ messages in thread From: Ruediger Meier @ 2016-02-12 16:23 UTC (permalink / raw) To: util-linux 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 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH] tests: mkfs.ext3 image-file needs option -F 2016-02-12 16:23 ` [PATCH] tests: mkfs.ext3 image-file needs option -F Ruediger Meier @ 2016-02-12 16:25 ` Ruediger Meier 0 siblings, 0 replies; 10+ messages in thread From: Ruediger Meier @ 2016-02-12 16:25 UTC (permalink / raw) To: util-linux 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2016-02-12 16:25 UTC | newest] Thread overview: 10+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2015-07-02 0:03 script, broken test options/return Ruediger Meier 2015-07-02 8:23 ` Karel Zak 2015-07-02 8:32 ` Karel Zak 2015-07-02 10:06 ` Ruediger Meier 2015-07-02 10:10 ` [PATCH] script: evaluate errno only if read() sets it Ruediger Meier 2015-07-02 10:21 ` Ruediger Meier 2015-07-03 8:04 ` Karel Zak 2015-07-03 10:33 ` Ruediger Meier 2016-02-12 16:23 ` [PATCH] tests: mkfs.ext3 image-file needs option -F Ruediger Meier 2016-02-12 16:25 ` Ruediger Meier
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox