diff mbox series

[Openvpn-devel] CMake: fix broken daemonization and syslog functionality

Message ID 20230920121519.177949-1-frank@lichtenheld.com
State Accepted
Headers
Series [Openvpn-devel] CMake: fix broken daemonization and syslog functionality |

Commit Message

Frank Lichtenheld Sept. 20, 2023, 12:15 p.m. UTC 
  From: Lev Stipakov <lev@openvpn.net>

While CMake is not the official way to build OpenVPN on Linux,
it still make sense to support it. Turns out that

  HAVE_SETSID, HAVE_OPENLOG and HAVE_SYSLOG

were not set by CMake configure, and --daemon and syslog
functionality was broken.

While on it, fix compiler error on unused return value of chdir().

Change-Id: I171d55da2be868d961caa1d4491e6f1ed10ebe8a
Signed-off-by: Lev Stipakov <lev@openvpn.net>
Acked-by: Arne Schwabe <arne-openvpn@rfc2549.org>
Acked-by: Frank Lichtenheld <frank@lichtenheld.com>
---

This change was reviewed on Gerrit and approved by at least one
developer. I request to merge it to master.

Gerrit URL: https://gerrit.openvpn.net/c/openvpn/+/341
This mail reflects revision 1 of this Change.
Acked-by according to Gerrit (reflected above):
Arne Schwabe <arne-openvpn@rfc2549.org>
Frank Lichtenheld <frank@lichtenheld.com>

Comments

Frank Lichtenheld Sept. 20, 2023, 12:21 p.m. UTC | #1 
On Wed, Sep 20, 2023 at 02:15:19PM +0200, Frank Lichtenheld wrote:
> From: Lev Stipakov <lev@openvpn.net>
> 
> While CMake is not the official way to build OpenVPN on Linux,
> it still make sense to support it. Turns out that
> 
>   HAVE_SETSID, HAVE_OPENLOG and HAVE_SYSLOG
> 
> were not set by CMake configure, and --daemon and syslog
> functionality was broken.
> 
> While on it, fix compiler error on unused return value of chdir().
> 
> Change-Id: I171d55da2be868d961caa1d4491e6f1ed10ebe8a
> Signed-off-by: Lev Stipakov <lev@openvpn.net>
> Acked-by: Arne Schwabe <arne-openvpn@rfc2549.org>
> Acked-by: Frank Lichtenheld <frank@lichtenheld.com>
> ---
> 
> This change was reviewed on Gerrit and approved by at least one
> developer. I request to merge it to master.
> 
> Gerrit URL: https://gerrit.openvpn.net/c/openvpn/+/341
> This mail reflects revision 1 of this Change.
> Acked-by according to Gerrit (reflected above):
> Arne Schwabe <arne-openvpn@rfc2549.org>
> Frank Lichtenheld <frank@lichtenheld.com>

So I can automatically set In-Reply-To to the same "virtual" message
ID that Gerrit uses (since that is deterministally constructed from
the change metadata). However, since this is not the actual message ID
by the original "newchange" mail, it still doesn't associate to the
same "Patch" in Patchwork. Instead it creates a new patch.

Regards,
Gert Doering Sept. 20, 2023, 8:35 p.m. UTC | #2 
Your patch has been applied to the master branch.

I have not tested it further, given that it has plenty of ACKs (and
only affects cmake build system).

Two observations, though - the cmake changes look incomplete, there
is still many "#undef HAVE_something" left.  Shouldn't they all be
completed to "#cmakedefine" for this to make sense?

Second, while not really relevant on Linux or *BSD, erroring-out
in daemon() on chdir("/") fail is not what either manpage documents -
and I do not think chdir("/") (as root) can ever fail.  So changing the
code flow just to silence the compiler feels a bit meh - doing a (void)
there would have silenced it just fine.  But as I said it's not 
truly relevant.

What OS were you building anyway that you hit an error in compat-daemon.c
(aka "are we missing a cmake test for daemon()")?

commit 8ae6c48d5d52dec8ec6e47cc1cfe89de9f2ffbcd (master)
Author: Lev Stipakov
Date:   Wed Sep 20 14:15:19 2023 +0200

     CMake: fix broken daemonization and syslog functionality

     Signed-off-by: Lev Stipakov <lev@openvpn.net>
     Acked-by: Arne Schwabe <arne-openvpn@rfc2549.org>
     Acked-by: Frank Lichtenheld <frank@lichtenheld.com>
     Message-Id: <20230920121519.177949-1-frank@lichtenheld.com>
     URL: https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg27045.html
     Signed-off-by: Gert Doering <gert@greenie.muc.de>


--
kind regards,

Gert Doering
diff mbox series

Patch

diff --git a/CMakeLists.txt b/CMakeLists.txt
index 7dae665..9de6aba 100644
--- a/CMakeLists.txt
+++ b/CMakeLists.txt
@@ -150,6 +150,7 @@ 
 check_symbol_exists(ftruncate unistd.h HAVE_FTRUNCATE)
 check_symbol_exists(setgid unistd.h HAVE_SETGID)
 check_symbol_exists(setuid unistd.h HAVE_SETUID)
+check_symbol_exists(setsid unistd.h HAVE_SETSID)
 check_symbol_exists(getpeereid unistd.h HAVE_GETPEEREID)
 
 check_symbol_exists(epoll_create sys/epoll.h HAVE_EPOLL_CREATE)
diff --git a/config.h.cmake.in b/config.h.cmake.in
index 4f28917..f2cdd39 100644
--- a/config.h.cmake.in
+++ b/config.h.cmake.in
@@ -218,7 +218,7 @@ 
 #undef HAVE_NICE
 
 /* Define to 1 if you have the `openlog' function. */
-#undef HAVE_OPENLOG
+#cmakedefine HAVE_OPENLOG
 
 /* OpenSSL engine support available */
 #undef HAVE_OPENSSL_ENGINE
@@ -258,7 +258,7 @@ 
 #undef HAVE_SETGROUPS
 
 /* Define to 1 if you have the `setsid' function. */
-#undef HAVE_SETSID
+#cmakedefine HAVE_SETSID
 
 /* Define to 1 if you have the `setsockopt' function. */
 #define HAVE_SETSOCKOPT 1
@@ -303,7 +303,7 @@ 
 #undef HAVE_STRSEP
 
 /* Define to 1 if you have the `syslog' function. */
-#undef HAVE_SYSLOG
+#cmakedefine HAVE_SYSLOG
 
 /* Define to 1 if you have the <syslog.h> header file. */
 #cmakedefine HAVE_SYSLOG_H
diff --git a/src/compat/compat-daemon.c b/src/compat/compat-daemon.c
index aebb8f4..5c6d740 100644
--- a/src/compat/compat-daemon.c
+++ b/src/compat/compat-daemon.c
@@ -70,7 +70,10 @@ 
 
     if (!nochdir)
     {
-        chdir("/");
+        if (chdir("/") == -1)
+        {
+            return (-1);
+        }
     }
 
     if (!noclose)