Re: [Quilt-dev] Race in test suite (faildiff.test)

[Martin Quinson]

Hello, 

sorry for the noise of self-answer, and for the top-posting.

I've spent some times to read 'run' in more details, and this code is
really more complex that it ought to be. In exec_test(), we are:
- opening 2 pipes
- saving away both STDIN and STDOUT to temporary file handlers
- replacing STDIN and STDOUT with some ends of the pipes
Then, if we are the parent process, we realize we should not have
changed STDIN and STDOUT so we are restoring them to their saved
value. It would be much clearer to only change STDIN and STDERR in the
child process and not mess with it in the other case.

We should also try harder to close all the file descriptors that we
are opening. I've been teaching pipes and forks for over 10 years now
(although in C, not in perl) and my experience is that bad things will
bite thy if you let some open pipes flying around. This of course
would be easier if we would not have to dupplicate STDIN and STDERR to
restore them when we notice we should not have changed them in the
first place.

I am wondering whether the bug lays in that line that I already
pointed out:     open STDERR, ">&STDOUT"
I think it should read as follows, even if I never really understood
the typeglob thing in Perl. At least for sake of consistency it
should read:     open *STDERR, ">&STDOUT"
Well, no. If that were the problem, STDERR would not be captured at
all when not adding 2>&1 to the shell. So I guess we should add * for
sake of consistency only. I hope it's not breaking anything either. 

My last idea would be to move the STDOUT->autoflush() to after the
redirection of STDERR (and also pass the optional 1 to autoflush()
just to make clear what we are trying to achieve). This should have no
effect at the system level, but there is maybe some oddies in Perl.

I know I should do the testing myself, but run is probably the most
intimidating part of quilt, which already undermine my hacking
self-confidence these days... Jean, would you mind sharing how you're
testing whether a given change has a positive impact on that race
condition? 

Thanks, Mt.

On Fri, Jan 26, 2018 at 04:22:09AM +0100, Martin Quinson wrote:
> Hello,
> 
> On Thu, Jan 25, 2018 at 11:11:18PM +0100, Jean Delvare wrote:
> > > Try appending a "2>&1" to direct all output to a single file.
> > 
> > Something like:
> > 
> > --- a/test/faildiff.test
> > +++ b/test/faildiff.test
> > @@ -27,7 +27,7 @@ What happens on binary files?
> >     > File test.bin added to patch %{P}test.diff
> >  
> >     $ printf "\\003\\000\\001" > test.bin
> > -   $ quilt diff -pab --no-index
> > +   $ quilt diff -pab --no-index 2>&1
> >     >~ (Files|Binary files) a/test\.bin and b/test\.bin differ
> >     > Diff failed on file 'test.bin', aborting
> >     $ echo %{?}
> > 
> > I tried that already, but it doesn't help. Which doesn't really
> > surprise me: while the code in test/run is a bit beyond my own perl-fu,
> > I seem to understand that the script itself is treating stdin and stdout
> > all the same, so it would make no difference if we merge them in the
> > test case itself.
> > 
> > That, on the other hand, works around the problem:
> > 
> > --- a/test/faildiff.test
> > +++ b/test/faildiff.test
> > @@ -27,8 +27,9 @@ What happens on binary files?
> >     > File test.bin added to patch %{P}test.diff
> >  
> >     $ printf "\\003\\000\\001" > test.bin
> > -   $ quilt diff -pab --no-index
> > +   $ quilt diff -pab --no-index 2>/dev/null
> >     >~ (Files|Binary files) a/test\.bin and b/test\.bin differ
> > +   $ quilt diff -pab --no-index 1>/dev/null
> >     > Diff failed on file 'test.bin', aborting
> >     $ echo %{?}
> >     > 1
> > 
> > But isn't it more of a hack than an actual fix? Aren't stdout and
> > stderr supposed to be line-buffered? And when pointing to the same file
> > or tty, shouldn't the messages reach said file or tty in the same order
> > as printed in the code?
> 
> Well, I would say that stdout is line-buffered as long as it's sent to
> a tty, but if it gets onto a pipe it's not line-buffered anymore. But
> I still don't get why 2>&1 does not helps here. I cannot fully
> understand the details of the perl code either, but it seems to me
> like there is too much pipes going on in the exec_test() function. 
> 
> For example, why are we duplicating STDERR in the client code since
> we're never using the copy? Also, with the following line getting
> executed by the child process right after duplicating STDERR, I don't
> see how pipe redirection at the shell level could have any impact. 
> 
>                 open STDERR, ">&STDOUT"
>               
> But from your tests it does. This is puzzling.
> 
> Bye, Mt.
> 
> -- 
> Let's call it an accidental feature.  -- Larry Wall



> _______________________________________________
> Quilt-dev mailing list
> address@hidden
> https://lists.nongnu.org/mailman/listinfo/quilt-dev


-- 
Il y a 10 catégories de gens : ceux qui ignorent tout du binaire, les
informaticiens, et ceux qui s'attendaient à du ternaire ici.

signature.asc
Description: PGP signature