Re: [PATCH v3] Rework the jobserver open logic

[Mauro Carvalho Chehab <mchehab+huawei@kernel.org>]

Em Fri, 16 Jan 2026 10:39:19 -0700
Jonathan Corbet <corbet@lwn.net> escreveu:

> The parsing of jobserver options is done in a massive try: block that hides
> problems and (perhaps) bugs.  Split up that block and make the logic
> explicit by moving the initial parsing of MAKEFLAGS out of that block.  Add
> warnings in the places things can go wrong.
> 
> Reviewed-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>
> Signed-off-by: Jonathan Corbet <corbet@lwn.net>
> ---
> Changes in v3:
>   - Let warn() take multiple arguments
>   - Fix syntactic brown-paper-bag error
> 
>  tools/lib/python/jobserver.py | 143 +++++++++++++++++++++-------------
>  1 file changed, 90 insertions(+), 53 deletions(-)
> 
> diff --git a/tools/lib/python/jobserver.py b/tools/lib/python/jobserver.py
> index 616411087725..7659b93fd566 100755
> --- a/tools/lib/python/jobserver.py
> +++ b/tools/lib/python/jobserver.py
> @@ -35,6 +35,9 @@ import os
>  import subprocess
>  import sys
>  
> +def warn(text, *args):
> +    print(f'WARNING: {text}', *args, file = sys.stderr)

Nit:
pylint would complain about that: it should be "file=sys.stderr".

Heh, we should really write a coding style for python and define
what linter we would use, if any ;-)

> +
>  class JobserverExec:
>      """
>      Claim all slots from make using POSIX Jobserver.
> @@ -58,64 +61,98 @@ class JobserverExec:
>  
>          if self.is_open:
>              return
> -
> -        try:
> -            # Fetch the make environment options.
> -            flags = os.environ["MAKEFLAGS"]
> -            # Look for "--jobserver=R,W"
> -            # Note that GNU Make has used --jobserver-fds and --jobserver-auth
> -            # so this handles all of them.
> -            opts = [x for x in flags.split(" ") if x.startswith("--jobserver")]
> -
> -            # Parse out R,W file descriptor numbers and set them nonblocking.
> -            # If the MAKEFLAGS variable contains multiple instances of the
> -            # --jobserver-auth= option, the last one is relevant.
> -            fds = opts[-1].split("=", 1)[1]
> -
> -            # Starting with GNU Make 4.4, named pipes are used for reader
> -            # and writer.
> -            # Example argument: --jobserver-auth=fifo:/tmp/GMfifo8134
> -            _, _, path = fds.partition("fifo:")
> -
> -            if path:
> +        self.is_open = True  # We only try once
> +        self.claim = None
> +        #
> +        # Check the make flags for "--jobserver=R,W"
> +        # Note that GNU Make has used --jobserver-fds and --jobserver-auth
> +        # so this handles all of them.
> +        #
> +        flags = os.environ.get('MAKEFLAGS', '')
> +        opts = [x for x in flags.split(" ") if x.startswith("--jobserver")]
> +        if not opts:
> +            return
> +        #
> +        # Separate out the provided file descriptors
> +        #
> +        split_opt = opts[-1].split('=', 1)
> +        if len(split_opt) != 2:
> +            warn('unparseable option:', opts[-1])
> +            return
> +        fds = split_opt[1]
> +        #
> +        # As of GNU Make 4.4, we'll be looking for a named pipe
> +        # identified as fifo:path
> +        #
> +        if fds.startswith('fifo:'):
> +            path = fds[len('fifo:'):]
> +            try:
>                  self.reader = os.open(path, os.O_RDONLY | os.O_NONBLOCK)
>                  self.writer = os.open(path, os.O_WRONLY)
> -            else:
> -                self.reader, self.writer = [int(x) for x in fds.split(",", 1)]
> +            except (OSError, IOError):
> +                warn('unable to open jobserver pipe', path)
> +                return
> +        #
> +        # Otherwise look for integer file-descriptor numbers.
> +        #
> +        else:
> +            split_fds = fds.split(',')
> +            if len(split_fds) != 2:
> +                warn('malformed jobserver file descriptors:', fds)
> +                return
> +            try:
> +                self.reader = int(split_fds[0])
> +                self.writer = int(split_fds[1])
> +            except ValueError:
> +                warn('non-integer jobserver file-descriptors:', fds)
> +                return
> +            try:
> +                #
>                  # Open a private copy of reader to avoid setting nonblocking
>                  # on an unexpecting process with the same reader fd.
> -                self.reader = os.open("/proc/self/fd/%d" % (self.reader),
> +                #
> +                self.reader = os.open(f"/proc/self/fd/{self.reader}",
>                                        os.O_RDONLY | os.O_NONBLOCK)
> -
> -            # Read out as many jobserver slots as possible
> -            while True:
> -                try:
> -                    slot = os.read(self.reader, 8)
> -                    if not slot:
> -                        # Clear self.jobs to prevent us from probably writing incorrect file.
> -                        self.jobs = b""
> -                        raise ValueError("unexpected empty token from jobserver fd, invalid '--jobserver-auth=' setting?")
> -                    self.jobs += slot
> -                except (OSError, IOError) as e:
> -                    if e.errno == errno.EWOULDBLOCK:
> -                        # Stop at the end of the jobserver queue.
> -                        break
> -                    # If something went wrong, give back the jobs.
> -                    if self.jobs:
> -                        os.write(self.writer, self.jobs)
> -                    raise e
> -
> -            # Add a bump for our caller's reserveration, since we're just going
> -            # to sit here blocked on our child.
> -            self.claim = len(self.jobs) + 1
> -
> -        except (KeyError, IndexError, ValueError, OSError, IOError) as e:
> -            print(f"jobserver: warning: {repr(e)}", file=sys.stderr)
> -            # Any missing environment strings or bad fds should result in just
> -            # not being parallel.
> -            self.claim = None
> -
> -        self.is_open = True
> +            except (IOError, OSError):
> +                warn('Unable to reopen jobserver read-side pipe')

I would add {repr(e)} here, to allow debugging what error was raised:

	warn(f"Unable to reopen jobserver read-side pipe: {repr(e)}")

(or pass it via args, as the logic below)

> +                return
> +        #
> +        # OK, we have the channel to the job server; read out as many jobserver
> +        # slots as possible.
> +        #
> +        while True:
> +            try:
> +                slot = os.read(self.reader, 8)
> +                if not slot:
> +                    #
> +                    # Something went wrong.  Clear self.jobs to avoid writing
> +                    # weirdness back to the jobserver and give up.
> +                    self.jobs = b""
> +                    warn("unexpected empty token from jobserver;"
> +                         " possible invalid '--jobserver-auth=' setting")

Maybe you should print "opts" here as well.

> +                    self.claim = None
> +                    return
> +            except (OSError, IOError) as e:
> +                #
> +                # If there is nothing more to read then we are done.
> +                #
> +                if e.errno == errno.EWOULDBLOCK:
> +                    break
> +                #
> +                # Anything else says that something went weird; give back
> +                # the jobs and give up.
> +                #
> +                if self.jobs:
> +                    os.write(self.writer, self.jobs)
> +                    self.claim = None
> +                    warn('error reading from jobserver pipe', e)
> +                    return
> +            self.jobs += slot
> +        #
> +        # Add a bump for our caller's reserveration, since we're just going
> +        # to sit here blocked on our child.
> +        #
> +        self.claim = len(self.jobs) + 1
>  
>      def close(self):
>          """Return all reserved slots to Jobserver"""



Thanks,
Mauro