diff mbox series

[Openvpn-devel,v2] Fix StatusChangeCallback so it works without a LogCallback

Message ID 20230906171758.344862-1-jeremyfleischman@gmail.com
State Accepted
Delegated to: David Sommerseth
Headers show
Series [Openvpn-devel,v2] Fix StatusChangeCallback so it works without a LogCallback | expand

Commit Message

Jeremy Fleischman Sept. 6, 2023, 5:17 p.m. UTC 
`StatusChangeCallback` requires that LogForward be enabled, which
previously only happened in `LogCallback`. Now both of them do it, which
requires a bit of bookkeeping. This fixes
https://github.com/OpenVPN/openvpn3-linux/issues/197

Signed-off-by: Jeremy Fleischman <jeremyfleischman@gmail.com>
---
 src/python/openvpn3/SessionManager.py | 69 ++++++++++++++++++++++-----
 1 file changed, 58 insertions(+), 11 deletions(-)

Comments

David Sommerseth Sept. 11, 2023, 8:57 a.m. UTC | #1 
On 06/09/2023 19:17, Jeremy Fleischman wrote:
> `StatusChangeCallback` requires that LogForward be enabled, which
> previously only happened in `LogCallback`. Now both of them do it, which
> requires a bit of bookkeeping. This fixes
> https://github.com/OpenVPN/openvpn3-linux/issues/197
> 
> Signed-off-by: Jeremy Fleischman <jeremyfleischman@gmail.com>
> ---
>   src/python/openvpn3/SessionManager.py | 69 ++++++++++++++++++++++-----
>   1 file changed, 58 insertions(+), 11 deletions(-)
> 

Thanks a lot for your patience and persistence through this review.

Your patch has been put into the queue for the coming v21 release.  I've 
not yet pushed out that branch, as I'm waiting for our QA team to 
complete the testing of an updated OpenVPN 3 Core library release first; 
which is required to be able to build OpenVPN 3 Linux from the public repos.

But consider your patch applied.  I've only done minor edits to some 
comments and commit messages, but the code itself is unchanged.

I'll follow-up with an update once this commit is public.
Jeremy Fleischman Sept. 11, 2023, 9 a.m. UTC | #2 
Thank you for the update, and patience. My first email-based patch :)

On Mon, Sep 11, 2023, 1:57 AM David Sommerseth <dazo+openvpn@eurephia.org>
wrote:

> On 06/09/2023 19:17, Jeremy Fleischman wrote:
> > `StatusChangeCallback` requires that LogForward be enabled, which
> > previously only happened in `LogCallback`. Now both of them do it, which
> > requires a bit of bookkeeping. This fixes
> > https://github.com/OpenVPN/openvpn3-linux/issues/197
> >
> > Signed-off-by: Jeremy Fleischman <jeremyfleischman@gmail.com>
> > ---
> >   src/python/openvpn3/SessionManager.py | 69 ++++++++++++++++++++++-----
> >   1 file changed, 58 insertions(+), 11 deletions(-)
> >
>
> Thanks a lot for your patience and persistence through this review.
>
> Your patch has been put into the queue for the coming v21 release.  I've
> not yet pushed out that branch, as I'm waiting for our QA team to
> complete the testing of an updated OpenVPN 3 Core library release first;
> which is required to be able to build OpenVPN 3 Linux from the public
> repos.
>
> But consider your patch applied.  I've only done minor edits to some
> comments and commit messages, but the code itself is unchanged.
>
> I'll follow-up with an update once this commit is public.
>
>
> --
> kind regards,
>
> David Sommerseth
> OpenVPN Inc
>
>
>
diff mbox series

Patch

diff --git a/src/python/openvpn3/SessionManager.py b/src/python/openvpn3/SessionManager.py
index 3632790..a175015 100644
--- a/src/python/openvpn3/SessionManager.py
+++ b/src/python/openvpn3/SessionManager.py
@@ -114,6 +114,7 @@  def __init__(self, dbuscon, objpath):
         self.__log_callback = None
         self.__status_callback = None
         self.__deleted = False
+        self.__log_forward_enabled = False
 
 
     def __del__(self):
@@ -284,6 +285,16 @@  def GetFormattedStatistics(self, prefix='Connection statistics:\n', format_str='
     #
     #
     def LogCallback(self, cbfnc):
+        if 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 RuntimeError('LogCallback() is already enabled')
+
+        if cbfnc is None and self.__log_callback is None:
+            # This is fine: disabling a callback when there is no callback is a
+            # simple no-op.
+            return
+
         if cbfnc is not None:
             self.__log_callback = cbfnc
             self.__dbuscon.add_signal_receiver(cbfnc,
@@ -291,16 +302,14 @@  def LogCallback(self, cbfnc):
                                                dbus_interface='net.openvpn.v3.backends',
                                                bus_name='net.openvpn.v3.log',
                                                path=self.__session_path)
-            self.__session_intf.LogForward(True)
         else:
-            try:
-                self.__session_intf.LogForward(False)
-            except dbus.exceptions.DBusException:
-                # If this fails, the session is typically already removed
-                pass
-            self.__dbuscon.remove_signal_receiver(self.__log_callback, 'Log')
-            self.__log_callback = None
+            # Only remove the callback if there actually *is* a callback
+            # currently.
+            if self.__log_callback is not None:
+                self.__dbuscon.remove_signal_receiver(self.__log_callback, 'Log')
+                self.__log_callback = None
 
+        self.__set_log_forward()
 
     ##
     #  Subscribes to the StatusChange signals for this session and register
@@ -310,6 +319,16 @@  def LogCallback(self, cbfnc):
     #     (integer) StatusMajor, (integer) StatusMinor, (string) message
     #
     def StatusChangeCallback(self, cbfnc):
+        if cbfnc is not None and self.__status_callback is not None:
+            # In this case, the program must first disable the
+            # current StatusChangeCallback() before setting a new one.
+            raise RuntimeError('StatusChangeCallback() is already enabled')
+
+        if cbfnc is None and self.__status_callback is None:
+            # This is fine: disabling a callback when there is no callback is a
+            # simple no-op.
+            return
+
         if cbfnc is not None:
             self.__status_callback = cbfnc
             self.__dbuscon.add_signal_receiver(cbfnc,
@@ -318,10 +337,14 @@  def StatusChangeCallback(self, cbfnc):
                                                bus_name='net.openvpn.v3.log',
                                                path=self.__session_path)
         else:
-            self.__dbuscon.remove_signal_receiver(self.__status_callback,
-                                                  'StatusChange')
-            self.__status_callback = None
+            # Only remove the callback if there actually *is* a callback
+            # currently.
+            if self.__status_callback is not None:
+                self.__dbuscon.remove_signal_receiver(self.__status_callback,
+                                                      'StatusChange')
+                self.__status_callback = None
 
+        self.__set_log_forward()
 
 
     ##
@@ -417,6 +440,30 @@  def GetDCO(self):
     def SetDCO(self, dco):
         self.__prop_intf.Set('net.openvpn.v3.sessions', 'dco', dco)
 
+    ##
+    #  Internal method to enable/disable LogForward as needed.
+    #  Must be called whenever a callback that needs LogForward enabled is
+    #  added or removed.
+    #
+    def __set_log_forward(self):
+        # The LogCallback and the StatusChangeCallback both need LogForward
+        # enabled. In other words, LogForward should be enabled iff one or both
+        # of those callbacks are registered.
+        should_log_forward_be_enabled = (
+            self.__log_callback is not None or self.__status_callback is not None
+        )
+
+        if should_log_forward_be_enabled and not self.__log_forward_enabled:
+            self.__session_intf.LogForward(True)
+            self.__log_forward_enabled = True
+        elif not should_log_forward_be_enabled and self.__log_forward_enabled:
+            try:
+                self.__session_intf.LogForward(False)
+            except dbus.exceptions.DBusException:
+                # If this fails, the session is typically already removed
+                pass
+
+            self.__log_forward_enabled = False
 
 
 ##