[PATCH] Rework the jobserver open logic

[PATCH] Rework the jobserver open logic

[Jonathan Corbet]

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.

Signed-off-by: Jonathan Corbet <corbet@lwn.net>
---
 tools/lib/python/jobserver.py | 140 +++++++++++++++++++++-------------
 1 file changed, 87 insertions(+), 53 deletions(-)

diff --git a/tools/lib/python/jobserver.py b/tools/lib/python/jobserver.py
index 616411087725e..cd3bddb6e7560 100755
--- a/tools/lib/python/jobserver.py
+++ b/tools/lib/python/jobserver.py
@@ -58,64 +58,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:
+            print('WARNING: 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[5:]
+            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):
+                print('WARNING: unable to open jobserver pipe', path)
+                return
+        #
+        # Otherwise look for integer file-descriptor numbers.
+        #
+        else:
+            split_fds = fds.split(',')
+            if len(split_fds) != 2:
+                print('WARNING: malformed jobserver file descriptors:', fds)
+                return
+            try:
+                self.reader = int(split_fds[0])
+                self.writer = int(split_fds[1])
+            except ValueError:
+                print('WARNING: 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):
+                print('WARNING: Unable to reopen jobserver read-side pipe')
+                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""
+                    print("WARNING: unexpected empty token from jobserver;"
+                          " possible invalid '--jobserver-auth=' setting")
+                    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
+                    print('WARNING: 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"""
-- 
2.52.0

Re: [PATCH] Rework the jobserver open logic

[Mauro Carvalho Chehab]

Em Tue, 13 Jan 2026 13:42:16 -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.
> 
> Signed-off-by: Jonathan Corbet <corbet@lwn.net>
> ---
>  tools/lib/python/jobserver.py | 140 +++++++++++++++++++++-------------
>  1 file changed, 87 insertions(+), 53 deletions(-)
> 
> diff --git a/tools/lib/python/jobserver.py b/tools/lib/python/jobserver.py
> index 616411087725e..cd3bddb6e7560 100755
> --- a/tools/lib/python/jobserver.py
> +++ b/tools/lib/python/jobserver.py
> @@ -58,64 +58,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:
> +            print('WARNING: unparseable option:', opts[-1])

On all prints, please use file=sys.stderr.

> +            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[5:]

Hmm... moving from partition() to startswith might be problematic...
are we sure that "fifo:" can't be in the middle of the string?

If so, instead of using "5" magic number, I would use len("fifo:")

Except for that, the changes looked OK on my eyes (but I didn't
test).

After addressing the above:

Reviewed-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>

Thanks,
Mauro

Re: [PATCH] Rework the jobserver open logic

[Jonathan Corbet]

Mauro Carvalho Chehab <mchehab+huawei@kernel.org> writes:

> On all prints, please use file=sys.stderr.

I was thinking, as I wrote all those, that I needed a warn() function,
but then it slipped my mind.

>> +            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[5:]
>
> Hmm... moving from partition() to startswith might be problematic...
> are we sure that "fifo:" can't be in the middle of the string?

If we don't get fifo:file then it's something unexpected and it should
complain.  At least with these changes it will actually complain.

> If so, instead of using "5" magic number, I would use len("fifo:")

I guess, though that's just replacing one constant with another; if
something changes, both have to be tweaked either way.

Thanks,

jon

Re: [PATCH] Rework the jobserver open logic

[Mauro Carvalho Chehab]

Em Wed, 14 Jan 2026 09:25:35 -0700
Jonathan Corbet <corbet@lwn.net> escreveu:

> Mauro Carvalho Chehab <mchehab+huawei@kernel.org> writes:
> 
> > On all prints, please use file=sys.stderr.  
> 
> I was thinking, as I wrote all those, that I needed a warn() function,
> but then it slipped my mind.

well, you could logging. As this is called also inside kernel-doc,
this could be a good idea.

> 
> >> +            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[5:]  
> >
> > Hmm... moving from partition() to startswith might be problematic...
> > are we sure that "fifo:" can't be in the middle of the string?  
> 
> If we don't get fifo:file then it's something unexpected and it should
> complain.  At least with these changes it will actually complain.
> 
> > If so, instead of using "5" magic number, I would use len("fifo:")  
> 
> I guess, though that's just replacing one constant with another; if
> something changes, both have to be tweaked either way.

I meant:

	FIFO = "fifo:"
	if fds.startswith(FIFO):
	    path = fds[len(FIFO):]

(or encapsulate it on a function)

> 
> Thanks,
> 
> jon



Thanks,
Mauro