Revert "vircommand: Parse /dev/fd on *BSD-like systems when looking for opened FDs"

[PATCH] Revert "vircommand: Parse /dev/fd on *BSD-like systems when looking for opened FDs"

Posted by Michal Privoznik 2 weeks, 5 days ago

Unfortunately, devfs on FreeBSD (accessible via /dev/fd) exposes
only those FDs which can be represented as a file. To cite
manpage [1]:

  The files /dev/fd/0 through /dev/fd/# refer to file descriptors
  which can be accessed through the file system.

This means FDs representing pipes and/or unnamed sockets are not
visible by default. To expose all FDs a slightly different
filesystem must be mounted [2]:

  mount -t fdescfs none /dev/fd

Apparently, on my test machine fdescfs is mounted by default and
thus I haven't seen any problem. Only after aforementioned patch
was merged our CI started reporting problems. While we could try
to figure out whether correct FS is mounted, it's a needless
micro optimization. Just revert the code to the state it was
before I touched it.

1: https://man.freebsd.org/cgi/man.cgi?query=fd&sektion=4&manpath=freebsd-release-ports
2: https://man.freebsd.org/cgi/man.cgi?query=fdescfs&sektion=5&n=1

This reverts commit 308ec0fb2c77f4867179f00c628f05d1d784f370.

Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
---
 src/util/vircommand.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/src/util/vircommand.c b/src/util/vircommand.c
index 969d4c28ef..ea52acfbb8 100644
--- a/src/util/vircommand.c
+++ b/src/util/vircommand.c
@@ -472,7 +472,7 @@ virExecCommon(virCommand *cmd, gid_t *groups, int ngroups)
     return 0;
 }
 
-# if defined(__linux__) || defined(__APPLE__) || defined(__FreeBSD__)
+# ifdef __linux__
 static int
 virCommandMassCloseGetFDsDir(virBitmap *fds,
                              const char *dirName)
@@ -502,7 +502,7 @@ virCommandMassCloseGetFDsDir(virBitmap *fds,
 
     return 0;
 }
-# endif /* defined(__linux__) || defined(__APPLE__) || defined(__FreeBSD__) */
+# endif /* __linux__ */
 
 static int
 virCommandMassCloseGetFDs(virBitmap *fds)
@@ -513,8 +513,6 @@ virCommandMassCloseGetFDs(virBitmap *fds)
      * onto child process (well, the one we will exec soon since this
      * is called from the child). */
     return virCommandMassCloseGetFDsDir(fds, "/proc/self/fd");
-# elif defined(__APPLE__) || defined(__FreeBSD__)
-    return virCommandMassCloseGetFDsDir(fds, "/dev/fd");
 # else
     virBitmapSetAll(fds);
     return 0;
-- 
2.44.2

Re: [PATCH] Revert "vircommand: Parse /dev/fd on *BSD-like systems when looking for opened FDs"

Posted by Martin Kletzander 2 weeks, 5 days ago

On Mon, Sep 16, 2024 at 11:00:04AM +0200, Michal Privoznik wrote:
>Unfortunately, devfs on FreeBSD (accessible via /dev/fd) exposes
>only those FDs which can be represented as a file. To cite
>manpage [1]:
>
>  The files /dev/fd/0 through /dev/fd/# refer to file descriptors
>  which can be accessed through the file system.
>
>This means FDs representing pipes and/or unnamed sockets are not
>visible by default. To expose all FDs a slightly different
>filesystem must be mounted [2]:
>
>  mount -t fdescfs none /dev/fd
>
>Apparently, on my test machine fdescfs is mounted by default and
>thus I haven't seen any problem. Only after aforementioned patch
>was merged our CI started reporting problems. While we could try
>to figure out whether correct FS is mounted, it's a needless
>micro optimization. Just revert the code to the state it was
>before I touched it.
>
>1: https://man.freebsd.org/cgi/man.cgi?query=fd&sektion=4&manpath=freebsd-release-ports
>2: https://man.freebsd.org/cgi/man.cgi?query=fdescfs&sektion=5&n=1
>
>This reverts commit 308ec0fb2c77f4867179f00c628f05d1d784f370.
>
>Signed-off-by: Michal Privoznik <mprivozn@redhat.com>

Reviewed-by: Martin Kletzander <mkletzan@redhat.com>

>---
> src/util/vircommand.c | 6 ++----
> 1 file changed, 2 insertions(+), 4 deletions(-)
>
>diff --git a/src/util/vircommand.c b/src/util/vircommand.c
>index 969d4c28ef..ea52acfbb8 100644
>--- a/src/util/vircommand.c
>+++ b/src/util/vircommand.c
>@@ -472,7 +472,7 @@ virExecCommon(virCommand *cmd, gid_t *groups, int ngroups)
>     return 0;
> }
>
>-# if defined(__linux__) || defined(__APPLE__) || defined(__FreeBSD__)
>+# ifdef __linux__
> static int
> virCommandMassCloseGetFDsDir(virBitmap *fds,
>                              const char *dirName)
>@@ -502,7 +502,7 @@ virCommandMassCloseGetFDsDir(virBitmap *fds,
>
>     return 0;
> }
>-# endif /* defined(__linux__) || defined(__APPLE__) || defined(__FreeBSD__) */
>+# endif /* __linux__ */
>
> static int
> virCommandMassCloseGetFDs(virBitmap *fds)
>@@ -513,8 +513,6 @@ virCommandMassCloseGetFDs(virBitmap *fds)
>      * onto child process (well, the one we will exec soon since this
>      * is called from the child). */
>     return virCommandMassCloseGetFDsDir(fds, "/proc/self/fd");
>-# elif defined(__APPLE__) || defined(__FreeBSD__)
>-    return virCommandMassCloseGetFDsDir(fds, "/dev/fd");
> # else
>     virBitmapSetAll(fds);
>     return 0;
>-- 
>2.44.2
>