Re: [Openvpn-devel] [PATCH] Fix StatusChangeCallback so it works without a LogCallback
Hi Jeremy,
First of all; sorry it's taken so long time to get back to you. GH
issue #171 has unfortunately taken most of my time, so this patch went
on the side burner.
I've looked at your patch, and I wonder if it can be done a bit simpler.
I'm open to hear your views; I might have overlooked some details.
On 10/07/2023 01:19, Jeremy Fleischman wrote:
[...snip...]
@@ -285,22 +286,24 @@ def GetFormattedStatistics(self, prefix='Connection
statistics:\n', format_str='
#
def LogCallback(self, cbfnc):
if cbfnc is not None:
+# Remove the existing callback if there is one.
+if self.__log_callback is not None:
+self.LogCallback(None)
+
self.__log_callback = cbfnc
self.__dbuscon.add_signal_receiver(cbfnc,
signal_name='Log',
dbus_interface='net.openvpn.v3.backends',
bus_name='net.openvpn.v3.log',
path=self.__session_path)
-self.__session_intf.LogForward(True)
+self.__add_LogForward_receiver()
I'm wondering if this could be made cleared. The recursion is kinda
clever, but feels like it hides the purpose.
Wouldn't this code below work just as well (consider this more a concept
code than proper Python code)? Here there is no recursion and no
reference counting which could go wrong.
class Session(object):
[...]
self.__log_forward_enabled = False
def LogCallback(self, cbfnc):
if cbfnc is not None and self.__log_callback is None:
# Log Callback function is being enabled; not
# set before
self.__log_callback = cbfnc
self.__dbuscon.add_signal_receiver(cbfnc, ...)
self.__set_log_forward(True)
elif cbfnc is None and self.__log_callback is not None:
# Log Callback function is being disabled; can only
# happen because it was set
self.__dbuscon.remove_signal_receiver(self.__log_callback,
...)
self.__log_callback = None
try:
self.__set_log_forward(False)
except dbus.exception.DBusException:
pass
elif (cbfnc is not None and self.__log_callback is not None):
# In this case, the program must first disable the
# current LogCallback() before setting a new one.
raise RuntimeErrpr('LogCallback() is already enabled')
# No need to complain if unsetting an unset log callback
# function; this will not have any behavioral impact at all.
def __set_log_forward(self, enable):
# This method can only be called *after* callback
# function has been set in this object
if not self.__log_forward_enabled and enable:
# If log forwarding is disabled it can be enabled
self.__log_forward_enabled = True
self.__session_intf.LogForward(True)
elif self.__log_forward_enabled and not enable:
# Log forwarding can only be disabled if
# both Log and StatusChange callbacks are unset
if (self.__log_callback is None)
and (self.__status_callback is None):
self.__log_forward_enabled = False
self.__session_intf.LogForward(False)
The StatusChangeCallback() would need a similar implementation as
LogCallback() too.
In regards to multi-threading; I would not expect this code to be used
in a multi-threaded setup where different callbacks would be
enabled/disabled on the fly in parallel. But it might be good to add
this as a comment in general, that these methods are not considered
thread-safe. However, since the code snippets above does not use
reference counting, it should be a bit more robust as it bases the
decision on the value of the callback function pointers.
Thoughts?
--
kind regards,
David Sommerseth
OpenVPN Inc
___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel
Re: [Openvpn-devel] [PATCH] configure: disable engines if OPENSSL_NO_ENGINE is defined
Hi, On Sun, Sep 03, 2023 at 01:44:13PM -0700, orbea wrote: > Thank you, the patch was generated with 'git format-patch' so I would > think 'git am' can consume it, but please let me know if there is > anything else I can change. For the sake of the archives (so, nothing to do for you now) - what we usually do is, for a v2 of the patch, to tag it as such, and use git-send-email to send it $ git-send-email -v2 --in-reply-to=$original-message-id -1 this will make it show up with a nice "[PATCH v2]" on the list, threaded to the original v1. gert -- "If was one thing all people took for granted, was conviction that if you feed honest figures into a computer, honest figures come out. Never doubted it myself till I met a computer with a sense of humor." Robert A. Heinlein, The Moon is a Harsh Mistress Gert Doering - Munich, Germany g...@greenie.muc.de signature.asc Description: PGP signature ___ Openvpn-devel mailing list Openvpn-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openvpn-devel
Re: [Openvpn-devel] [PATCH] configure: disable engines if OPENSSL_NO_ENGINE is defined
On Sun, 3 Sep 2023 21:22:15 +0200 Gert Doering wrote: > Hi, > > On Sun, Sep 03, 2023 at 08:15:05PM +0200, Antonio Quartulli wrote: > > On 03/09/2023 18:55, orbea wrote: > > > Here is a patch that preserves the version check and adds a second > > > check for OPENSSL_NO_ENGINE which seems to also be useful for > > > BoringSSL. > > > > I prefer this version, but I'll let other chime in. > > On top of that I think we may need a proper v2 PATCH having no > > extra body, as I am not sure your message can be properly parsed by > > git-am. > > It should be fine, and if not, I'll massage it to make it go in. > > gert Thank you, the patch was generated with 'git format-patch' so I would think 'git am' can consume it, but please let me know if there is anything else I can change. ___ Openvpn-devel mailing list Openvpn-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openvpn-devel
Re: [Openvpn-devel] [PATCH] configure: disable engines if OPENSSL_NO_ENGINE is defined
Hi, On Sun, Sep 03, 2023 at 08:15:05PM +0200, Antonio Quartulli wrote: > On 03/09/2023 18:55, orbea wrote: > > Here is a patch that preserves the version check and adds a second > > check for OPENSSL_NO_ENGINE which seems to also be useful for BoringSSL. > > I prefer this version, but I'll let other chime in. > On top of that I think we may need a proper v2 PATCH having no extra body, > as I am not sure your message can be properly parsed by git-am. It should be fine, and if not, I'll massage it to make it go in. gert -- "If was one thing all people took for granted, was conviction that if you feed honest figures into a computer, honest figures come out. Never doubted it myself till I met a computer with a sense of humor." Robert A. Heinlein, The Moon is a Harsh Mistress Gert Doering - Munich, Germany g...@greenie.muc.de signature.asc Description: PGP signature ___ Openvpn-devel mailing list Openvpn-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openvpn-devel
Re: [Openvpn-devel] [PATCH] configure: disable engines if OPENSSL_NO_ENGINE is defined
Hi,
On 03/09/2023 18:55, orbea wrote:
Here is a patch that preserves the version check and adds a second
check for OPENSSL_NO_ENGINE which seems to also be useful for BoringSSL.
I prefer this version, but I'll let other chime in.
On top of that I think we may need a proper v2 PATCH having no extra
body, as I am not sure your message can be properly parsed by git-am.
Cheers,
From d6700ec0f5af2522bb4eb136d3760f5b1445c9d1 Mon Sep 17 00:00:00 2001
From: orbea
Date: Sat, 2 Sep 2023 23:06:22 -0700
Subject: [PATCH] configure: disable engines if OPENSSL_NO_ENGINE is defined
Starting with LibreSSL 3.8.1 the engines have been removed which causes
the OpenVPN build to fail. This can be solved during configure by
checking if OPENSSL_NO_ENGINE is defined in opensslconf.h.
---
configure.ac | 8 +++-
1 file changed, 7 insertions(+), 1 deletion(-)
diff --git a/configure.ac b/configure.ac
index 2f65cbd5..1adfb9d4 100644
--- a/configure.ac
+++ b/configure.ac
@@ -927,11 +927,17 @@ if test "${with_crypto_library}" = "openssl"; then
[AC_LANG_PROGRAM(
[[
#include
+ #include
]],
[[
/* Version encoding: MNNFFPPS - see opensslv.h for details */
#if OPENSSL_VERSION_NUMBER >= 0x3000L
- #error Engine supported disabled by default in OpenSSL 3.0+
+ #error Engine support disabled by default in OpenSSL 3.0+
+ #endif
+
+ /* BoringSSL and LibreSSL >= 3.8.1 removed engine support */
+ #ifdef OPENSSL_NO_ENGINE
+ #error Engine support disabled by default in openssl/opensslconf.h
#endif
]]
)],
--
Antonio Quartulli
___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel
Re: [Openvpn-devel] [PATCH] configure: disable engines if OPENSSL_NO_ENGINE is defined
On Sun, 3 Sep 2023 09:17:21 -0700
orbea wrote:
> On Sun, 3 Sep 2023 16:47:31 +0200
> Antonio Quartulli wrote:
>
> > Hi,
> >
> > On 03/09/2023 16:29, or...@riseup.net wrote:
> > > From: orbea
> > >
> > > Starting with LibreSSL 3.8.1 the engines have been removed which
> > > causes the OpenVPN build to fail. This can be solved during
> > > configure by checking if OPENSSL_NO_ENGINE is defined in
> > > opensslconf.h. ---
> > > configure.ac | 3 ++-
> > > 1 file changed, 2 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/configure.ac b/configure.ac
> > > index 2f65cbd5..b5a835dc 100644
> > > --- a/configure.ac
> > > +++ b/configure.ac
> > > @@ -926,11 +926,12 @@ if test "${with_crypto_library}" =
> > > "openssl"; then AC_COMPILE_IFELSE(
> > > [AC_LANG_PROGRAM(
> > > [[
> > > + #include
> > > #include
> > > ]],
> > > [[
> > > /* Version encoding: MNNFFPPS - see
> > > opensslv.h for details */
> > > - #if OPENSSL_VERSION_NUMBER >= 0x3000L
> > > + #if OPENSSL_VERSION_NUMBER >= 0x3000L ||
> > > defined(OPENSSL_NO_ENGINE) #error Engine supported disabled by
> > > default in OpenSSL 3.0+
> >
> > Maybe the message should be changed now? Or we could have an
> > entirely different message for this case?
> >
> > Cheers,
> >
> > > #endif
> > > ]]
> >
>
> Do you think it might be preferable to only check OPENSSL_NO_ENGINE? I
> see other code bases such as Tor only checking that define.
>
>
> ___
> Openvpn-devel mailing list
> Openvpn-devel@lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/openvpn-devel
Here is a patch that preserves the version check and adds a second
check for OPENSSL_NO_ENGINE which seems to also be useful for BoringSSL.
>From d6700ec0f5af2522bb4eb136d3760f5b1445c9d1 Mon Sep 17 00:00:00 2001
From: orbea
Date: Sat, 2 Sep 2023 23:06:22 -0700
Subject: [PATCH] configure: disable engines if OPENSSL_NO_ENGINE is defined
Starting with LibreSSL 3.8.1 the engines have been removed which causes
the OpenVPN build to fail. This can be solved during configure by
checking if OPENSSL_NO_ENGINE is defined in opensslconf.h.
---
configure.ac | 8 +++-
1 file changed, 7 insertions(+), 1 deletion(-)
diff --git a/configure.ac b/configure.ac
index 2f65cbd5..1adfb9d4 100644
--- a/configure.ac
+++ b/configure.ac
@@ -927,11 +927,17 @@ if test "${with_crypto_library}" = "openssl"; then
[AC_LANG_PROGRAM(
[[
#include
+ #include
]],
[[
/* Version encoding: MNNFFPPS - see opensslv.h for details */
#if OPENSSL_VERSION_NUMBER >= 0x3000L
- #error Engine supported disabled by default in OpenSSL 3.0+
+ #error Engine support disabled by default in OpenSSL 3.0+
+ #endif
+
+ /* BoringSSL and LibreSSL >= 3.8.1 removed engine support */
+ #ifdef OPENSSL_NO_ENGINE
+ #error Engine support disabled by default in openssl/opensslconf.h
#endif
]]
)],
--
2.41.0
___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel
Re: [Openvpn-devel] [PATCH] configure: disable engines if OPENSSL_NO_ENGINE is defined
On Sun, 3 Sep 2023 16:47:31 +0200
Antonio Quartulli wrote:
> Hi,
>
> On 03/09/2023 16:29, or...@riseup.net wrote:
> > From: orbea
> >
> > Starting with LibreSSL 3.8.1 the engines have been removed which
> > causes the OpenVPN build to fail. This can be solved during
> > configure by checking if OPENSSL_NO_ENGINE is defined in
> > opensslconf.h. ---
> > configure.ac | 3 ++-
> > 1 file changed, 2 insertions(+), 1 deletion(-)
> >
> > diff --git a/configure.ac b/configure.ac
> > index 2f65cbd5..b5a835dc 100644
> > --- a/configure.ac
> > +++ b/configure.ac
> > @@ -926,11 +926,12 @@ if test "${with_crypto_library}" = "openssl";
> > then AC_COMPILE_IFELSE(
> > [AC_LANG_PROGRAM(
> > [[
> > + #include
> > #include
> > ]],
> > [[
> > /* Version encoding: MNNFFPPS - see
> > opensslv.h for details */
> > - #if OPENSSL_VERSION_NUMBER >= 0x3000L
> > + #if OPENSSL_VERSION_NUMBER >= 0x3000L ||
> > defined(OPENSSL_NO_ENGINE) #error Engine supported disabled by
> > default in OpenSSL 3.0+
>
> Maybe the message should be changed now? Or we could have an entirely
> different message for this case?
>
> Cheers,
>
> > #endif
> > ]]
>
Do you think it might be preferable to only check OPENSSL_NO_ENGINE? I
see other code bases such as Tor only checking that define.
___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel
Re: [Openvpn-devel] [PATCH] configure: disable engines if OPENSSL_NO_ENGINE is defined
Hi,
On 03/09/2023 16:29, or...@riseup.net wrote:
From: orbea
Starting with LibreSSL 3.8.1 the engines have been removed which causes
the OpenVPN build to fail. This can be solved during configure by
checking if OPENSSL_NO_ENGINE is defined in opensslconf.h.
---
configure.ac | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/configure.ac b/configure.ac
index 2f65cbd5..b5a835dc 100644
--- a/configure.ac
+++ b/configure.ac
@@ -926,11 +926,12 @@ if test "${with_crypto_library}" = "openssl"; then
AC_COMPILE_IFELSE(
[AC_LANG_PROGRAM(
[[
+ #include
#include
]],
[[
/* Version encoding: MNNFFPPS - see opensslv.h for details */
- #if OPENSSL_VERSION_NUMBER >= 0x3000L
+ #if OPENSSL_VERSION_NUMBER >= 0x3000L ||
defined(OPENSSL_NO_ENGINE)
#error Engine supported disabled by default in OpenSSL 3.0+
Maybe the message should be changed now? Or we could have an entirely
different message for this case?
Cheers,
#endif
]]
--
Antonio Quartulli
___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel
[Openvpn-devel] [PATCH] configure: disable engines if OPENSSL_NO_ENGINE is defined
From: orbea
Starting with LibreSSL 3.8.1 the engines have been removed which causes
the OpenVPN build to fail. This can be solved during configure by
checking if OPENSSL_NO_ENGINE is defined in opensslconf.h.
---
configure.ac | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/configure.ac b/configure.ac
index 2f65cbd5..b5a835dc 100644
--- a/configure.ac
+++ b/configure.ac
@@ -926,11 +926,12 @@ if test "${with_crypto_library}" = "openssl"; then
AC_COMPILE_IFELSE(
[AC_LANG_PROGRAM(
[[
+ #include
#include
]],
[[
/* Version encoding: MNNFFPPS - see opensslv.h for details */
- #if OPENSSL_VERSION_NUMBER >= 0x3000L
+ #if OPENSSL_VERSION_NUMBER >= 0x3000L ||
defined(OPENSSL_NO_ENGINE)
#error Engine supported disabled by default in OpenSSL 3.0+
#endif
]]
--
2.41.0
___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel
