Re: [PATCH 2/6] isolation, conf: Set c->fd_tap from eary parse of --fd

[David Gibson <david@gibson.dropbear.id.au>]

[-- Attachment #1: Type: text/plain, Size: 2828 bytes --]

On Wed, Jul 15, 2026 at 09:12:35AM +0200, Stefano Brivio wrote:
> On Tue, 14 Jul 2026 19:29:22 +1000
> David Gibson <david@gibson.dropbear.id.au> wrote:
> 
> > We parse --fd twice: once in isolate_initial() just to avoid clobbering
> > the passed in fd.  Then we parse it "for real" in conf(), to set c->fd_tap
> > and other configuration variables.
> > 
> > Change this, so that we return the value parsed early from
> > isolate_initial() and set c->fd_tap from that.  This doesn't accomplish
> > much immediately, but will make some further cleanups possible.
> > 
> > Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> > ---
> >  conf.c      | 2 +-
> >  isolation.c | 6 +++++-
> >  isolation.h | 2 +-
> >  passt.c     | 2 +-
> >  4 files changed, 8 insertions(+), 4 deletions(-)
> > 
> > diff --git a/conf.c b/conf.c
> > index 6d83daef..41c9d557 100644
> > --- a/conf.c
> > +++ b/conf.c
> > @@ -1593,7 +1593,7 @@ void conf(struct ctx *c, int argc, char **argv)
> >  			c->fd_control_listen = c->fd_control = -1;
> >  			break;
> >  		case 'F':
> > -			c->fd_tap = conf_tap_fd(optarg);
> > +			/* --fd was parsed early and c->fd_tap set in main() */
> >  			c->one_off = true;
> >  			*c->sock_path = 0;
> >  			break;
> > diff --git a/isolation.c b/isolation.c
> > index 07b281bb..4bb79e34 100644
> > --- a/isolation.c
> > +++ b/isolation.c
> > @@ -203,8 +203,10 @@ static int move_root(void)
> >   *  - close all open files except for standard streams and the one from --fd
> >   * Mustn't:
> >   *  - remove filesystem access (we need to access files during setup)
> > + *
> > + * Return: fd number from --fd, or -1 if not specified
> 
> I think this is a rather weird value to return for a function that's
> called isolate_initial().

Agreed, but there are reasons..

> Are patches 1/2 and 2/2 really needed for the
> rest of the series? I don't quite see the connection.

1) (minor) The new semantics of conf_tap_fd() introduced in 3/6 work
   better (IMO) for its use in close_open_files(), but don't work for
   its reuse during the "main" arg parse.  Removing that second parse
   avoids the problem.

2) (major) 4/6 significantly simplifies the close_range() logic, but
   it involves *moving* the tap descriptor given with --fd (using
   dup2(2).  That means if we parsed it again, it would now be wrong.

I think the weirdness would also be mitigated, if close_open_files()
became isolate_fds(), and was called directly from main() rather than
via isolate_initial().
> 
> I'm still reviewing the rest of the series.
> 
> -- 
> Stefano
> 

-- 
David Gibson (he or they)	| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you, not the other way
				| around.
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]