So far, virCommandMassCloseGetFDsLinux() opens "/proc/self/fd",
iterates over it marking opened FDs in @fds bitmap. Well, we can
do the same on other systems (with altered path), like MacOS or
FreeBSD. Therefore, isolate dir iteration into a separate
function that accepts dir path as an argument.

Unfortunately, this function might be unused on some systems
(e.g. mingw), therefore mark it as such.

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

diff --git a/src/util/vircommand.c b/src/util/vircommand.c
index 8bd5e5e771..c0aab85c53 100644
--- a/src/util/vircommand.c
+++ b/src/util/vircommand.c
@@ -472,17 +472,12 @@ virExecCommon(virCommand *cmd, gid_t *groups, int ngroups)
     return 0;
 }
 
-# ifdef __linux__
-/* On Linux, we can utilize procfs and read the table of opened
- * FDs and selectively close only those FDs we don't want to pass
- * onto child process (well, the one we will exec soon since this
- * is called from the child). */
-static int
-virCommandMassCloseGetFDsLinux(virBitmap *fds)
+static int G_GNUC_UNUSED
+virCommandMassCloseGetFDsDir(virBitmap *fds,
+                             const char *dirName)
 {
     g_autoptr(DIR) dp = NULL;
     struct dirent *entry;
-    const char *dirName = "/proc/self/fd";
     int rc;
 
     if (virDirOpen(&dp, dirName) < 0)
@@ -507,15 +502,20 @@ virCommandMassCloseGetFDsLinux(virBitmap *fds)
     return 0;
 }
 
-# else /* !__linux__ */
-
 static int
-virCommandMassCloseGetFDsGeneric(virBitmap *fds)
+virCommandMassCloseGetFDs(virBitmap *fds)
 {
+# ifdef __linux__
+    /* On Linux, we can utilize procfs and read the table of opened
+     * FDs and selectively close only those FDs we don't want to pass
+     * onto child process (well, the one we will exec soon since this
+     * is called from the child). */
+    return virCommandMassCloseGetFDsDir(fds, "/proc/self/fd");
+# else
     virBitmapSetAll(fds);
     return 0;
+# endif
 }
-# endif /* !__linux__ */
 
 static int
 virCommandMassCloseFrom(virCommand *cmd,
@@ -544,13 +544,8 @@ virCommandMassCloseFrom(virCommand *cmd,
 
     fds = virBitmapNew(openmax);
 
-# ifdef __linux__
-    if (virCommandMassCloseGetFDsLinux(fds) < 0)
+    if (virCommandMassCloseGetFDs(fds) < 0)
         return -1;
-# else
-    if (virCommandMassCloseGetFDsGeneric(fds) < 0)
-        return -1;
-# endif
 
     lastfd = MAX(lastfd, childin);
     lastfd = MAX(lastfd, childout);
-- 
2.44.2

On Wed, Aug 28, 2024 at 02:16:15PM +0200, Michal Privoznik wrote:
>So far, virCommandMassCloseGetFDsLinux() opens "/proc/self/fd",
>iterates over it marking opened FDs in @fds bitmap. Well, we can
>do the same on other systems (with altered path), like MacOS or
>FreeBSD. Therefore, isolate dir iteration into a separate
>function that accepts dir path as an argument.
>
>Unfortunately, this function might be unused on some systems
>(e.g. mingw), therefore mark it as such.
>

Not in this patch.  This patch should leave it in without marking it as
unused.  With that

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

On 9/12/24 15:19, Martin Kletzander wrote:
> On Wed, Aug 28, 2024 at 02:16:15PM +0200, Michal Privoznik wrote:
>> So far, virCommandMassCloseGetFDsLinux() opens "/proc/self/fd",
>> iterates over it marking opened FDs in @fds bitmap. Well, we can
>> do the same on other systems (with altered path), like MacOS or
>> FreeBSD. Therefore, isolate dir iteration into a separate
>> function that accepts dir path as an argument.
>>
>> Unfortunately, this function might be unused on some systems
>> (e.g. mingw), therefore mark it as such.
>>
> 
> Not in this patch.  This patch should leave it in without marking it as
> unused.  With that

Why not? A new function is introduced, outside of any #ifdef block, but
it's used solely in #ifdef __linux__ (for now). Or am I missing something?

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

Michal

On Fri, Sep 13, 2024 at 09:37:30AM +0200, Michal Prívozník wrote:
>On 9/12/24 15:19, Martin Kletzander wrote:
>> On Wed, Aug 28, 2024 at 02:16:15PM +0200, Michal Privoznik wrote:
>>> So far, virCommandMassCloseGetFDsLinux() opens "/proc/self/fd",
>>> iterates over it marking opened FDs in @fds bitmap. Well, we can
>>> do the same on other systems (with altered path), like MacOS or
>>> FreeBSD. Therefore, isolate dir iteration into a separate
>>> function that accepts dir path as an argument.
>>>
>>> Unfortunately, this function might be unused on some systems
>>> (e.g. mingw), therefore mark it as such.
>>>
>>
>> Not in this patch.  This patch should leave it in without marking it as
>> unused.  With that
>
>Why not? A new function is introduced, outside of any #ifdef block, but
>it's used solely in #ifdef __linux__ (for now). Or am I missing something?
>

Yes, it is used only if __linux__ so it should be marked as such.  Later
patch should add the other systems.

The issue I have with marking it as unused is that it is then there
forever and we won't notice it being completely unused at some point.
And it does not feel too clean.

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