[RFC PATCH v3 7/9] libcamera: process: Move `closeAllFdsExcept()`

[Laurent Pinchart]

On Tue, Mar 25, 2025 at 07:09:37PM +0000, Kieran Bingham wrote:
> Quoting Barnabás Pőcze (2025-03-25 18:08:19)
> > Remove `closeAllFdsExcept()` from the `Process` class and have
> > it as a local function since it is no needed "outside" and does
> > not depend on any part of the `Process` class.
> 
> Any impact on https://patchwork.libcamera.org/patch/22403/ ? Could you
> review that please?
> 
> > 
> > Signed-off-by: Barnabás Pőcze <barnabas.pocze at ideasonboard.com>
> > ---
> >  include/libcamera/internal/process.h |  1 -
> >  src/libcamera/process.cpp            | 56 ++++++++++++++--------------
> >  2 files changed, 28 insertions(+), 29 deletions(-)
> > 
> > diff --git a/include/libcamera/internal/process.h b/include/libcamera/internal/process.h
> > index e4f5bb979..14391d60a 100644
> > --- a/include/libcamera/internal/process.h
> > +++ b/include/libcamera/internal/process.h
> > @@ -44,7 +44,6 @@ public:
> >  private:
> >         LIBCAMERA_DISABLE_COPY_AND_MOVE(Process)
> >  
> > -       void closeAllFdsExcept(Span<const int> fds);
> 
> It's already private though isn't it ? 
> 
> Is it just better to keep functions isolated if they aren't directly
> using the class ?

The rationale for not including members in the class definition, even as
private members, is two-fold:

- It avoids ABI breakages

- It avoids recompilation of users of the class

The former is only applicable to member data, and member virtual
functions. Adding, removing, or changing the signature of a non-virtual
member function doesn't affect the ABI.

The latter is true in all cases, but the practical impact is limited,
unless you routinely make modifications to a class whose header file is
widely included.

The downsides of avoiding private member functions is a decrease (in my
opinion) of readability. Making them non-member static functions
possibly requires passing member data to the function. It also decouples
the function from the class, which can make the code harder to follow.
In this specific case the function didn't use any member data, and the
readability isn't very negatively impacted. I am therefore not opposed
to this patch, even if I probably wouldn't have authored it. I think
it's partly a matter of personal preference.

> >         int isolate();
> >         void died(int wstatus);
> >  
> > diff --git a/src/libcamera/process.cpp b/src/libcamera/process.cpp
> > index 5603dc903..01503e485 100644
> > --- a/src/libcamera/process.cpp
> > +++ b/src/libcamera/process.cpp
> > @@ -63,6 +63,34 @@ void sigact(int signal, siginfo_t *info, void *ucontext)
> >         }
> >  }
> >  
> > +void closeAllFdsExcept(Span<const int> fds)
> 
> static? or is this already in an annoymous namespace or such ?

It's already in an anonymous namespace.

Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>

> > +{
> > +       std::vector<int> v(fds.begin(), fds.end());
> > +       std::sort(v.begin(), v.end());
> > +
> > +       ASSERT(v.empty() || v.front() >= 0);
> > +
> > +       DIR *dir = opendir("/proc/self/fd");
> > +       if (!dir)
> > +               return;
> > +
> > +       int dfd = dirfd(dir);
> > +
> > +       struct dirent *ent;
> > +       while ((ent = readdir(dir)) != nullptr) {
> > +               char *endp;
> > +               int fd = strtoul(ent->d_name, &endp, 10);
> > +               if (*endp)
> > +                       continue;
> > +
> > +               if (fd >= 0 && fd != dfd &&
> > +                   !std::binary_search(v.begin(), v.end(), fd))
> > +                       close(fd);
> > +       }
> > +
> > +       closedir(dir);
> > +}
> > +
> >  } /* namespace */
> >  
> >  void ProcessManager::sighandler()
> > @@ -282,34 +310,6 @@ int Process::start(const std::string &path,
> >         }
> >  }
> >  
> > -void Process::closeAllFdsExcept(Span<const int> fds)
> > -{
> > -       std::vector<int> v(fds.begin(), fds.end());
> > -       sort(v.begin(), v.end());
> > -
> > -       ASSERT(v.empty() || v.front() >= 0);
> > -
> > -       DIR *dir = opendir("/proc/self/fd");
> > -       if (!dir)
> > -               return;
> > -
> > -       int dfd = dirfd(dir);
> > -
> > -       struct dirent *ent;
> > -       while ((ent = readdir(dir)) != nullptr) {
> > -               char *endp;
> > -               int fd = strtoul(ent->d_name, &endp, 10);
> > -               if (*endp)
> > -                       continue;
> > -
> > -               if (fd >= 0 && fd != dfd &&
> > -                   !std::binary_search(v.begin(), v.end(), fd))
> > -                       close(fd);
> > -       }
> > -
> > -       closedir(dir);
> > -}
> > -
> >  int Process::isolate()
> >  {
> >         int ret = unshare(CLONE_NEWUSER | CLONE_NEWNET);

-- 
Regards,

Laurent Pinchart