Hey David,
On Sun, Sep 3, 2023 at 3:56 PM David Sommerseth
<dazo+open...@eurephia.org> wrote:
>
> 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.
No worries, thanks for getting around to this!
> 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.
I like this! At a high level, your proposal consolidates the logic
about "who needs LogForward enabled" into this new __set_log_forward
method. It maybe makes it a little more complicated to add more
callbacks in the future that require LogForward, but that's an
incredibly minor point, and I don't think it's worth the complication
of the reference counting implementation I originally proposed.
I took your idea, and rolled with it a bit. I think we can simplify
things a bit further if we let __set_log_forward be completely
responsible for determining if LogForward should be enabled (it knows
if it should be enabled because it can check if there are any
callbacks registered that require LogForward). How does this updated
diff look?
diff --git a/src/python/openvpn3/SessionManager.py
b/src/python/openvpn3/SessionManager.py
index 3632790..1a567be 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):
@@ -291,16 +292,11 @@ 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
+ self.__set_log_forward()
##
# Subscribes to the StatusChange signals for this session and register
@@ -318,10 +314,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 +417,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
##
_______________________________________________
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel
