On Fri, Oct 04, 2019 at 11:15:46AM +0200, Rasmus Villemoes wrote:
> On 25/09/2019 01.29, Kees Cook wrote:
> > +# Extract and prepare jobserver file descriptors from envirnoment.
> > +try:
> > + # Fetch the make environment options.
> > + flags = os.environ['MAKEFLAGS']
> > +
> > + # Look for "--jobserver=R,W"
> > + opts = [x for x in flags.split(" ") if x.startswith("--jobserver")]
>
> OK, this handles the fact that Make changed from --jobserver-fds to
> --jobserver-auth at some point. Probably the comment could be more accurate.
I can update that, sure.
> > +# Read out as many jobserver slots as possible.
> > +jobs = b""
> > +while True:
> > + try:
> > + slot = os.read(reader, 1)
> > + jobs += slot
> > + except (OSError, IOError) as e:
> > + if e.errno == errno.EWOULDBLOCK:
> > + # Stop when reach the end of the jobserver queue.
> > + break
> > + raise e
>
> Ah, reading more carefully you set O_NONBLOCK explicitly. Well, older
> Makes are going to be very unhappy about that (remember that it's a
> property of the file description and not file descriptor). They don't
> expect EAGAIN when fetching a token, so fail hard. Probably not when
> htmldocs is the only target, because in that case the toplevel Make just
> reads back the exact number of tokens it put in as a sanity check, but
> if it builds other targets/spawns other submakes, I think this breaks.
Do you mean the processes sharing the file will suddenly gain
O_NONBLOCK? I didn't think that was true, but I can test. If it is true,
we could easily just restore the state before exit.
> > +# Return all the reserved slots.
> > +os.write(writer, jobs)
>
> Well, that probably works ok for the isolated case of a toplevel "make
> -j12 htmldocs", but if you're building other targets ("make -j12
> htmldocs vmlinux") this will effectively inject however many tokens the
> above loop grabbed (which might not be all if the top-level make has
> started things related to the vmlinux target), so for the duration of
> the docs build, there will be more processes running than asked for.
That is true, yes, though I still think it's an improvement over the
existing case of sphinx-build getting run with -jauto which lands us in
the same (or worse) position.
The best solution would be to teach sphinx-build about the Make
jobserver, though I expect that would be weird. Another idea would be to
hold the reservation until sphinx-build finishes and THEN return the
slots? That would likely need to change from a utility to a sphinx-build
wrapper...
> > +# If the jobserver was (impossibly) full or communication failed, use default.
> > +if len(jobs) < 1:
> > + print(default)
> > +
> > +# Report available slots (with a bump for our caller's reserveration).
> > +print(len(jobs) + 1)
>
> There's a missing exit() or else: here; if len(jobs) < 1 you print both
> default (probably "1") and 0+1 aka "1".
Whoops! Yes, that needs fixing too. I'll send a patch...
--
Kees Cook
