On 04/09/2023 23:27, Jeremy Fleischman wrote:
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 RuntimeError('LogCallback() is already enabled')
This should enforce only one callback function being enabled this way,
and if the callback need to be changed - it's enough to first do disable
it (LogCallback(None)) before setting the new one. And if more
callbacks functions is wanted/needed, the additional ones can be called
via the callback function registered with the LogCallback(). No need to
make this code more complicated.
That makes sense! I *think* things get a little cleaner if we move this check
to the top of the relevant functions (StatusChangeCallback and LogCallback).
Basically, by first eliminating the possibility that we're changing an existing
registered dbus callback, then we don't actually need to be as careful later on.
For example, here's an updated version of LogCallback:
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 not 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)
else:
# 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()
Does this seem safe?
I have a feeling we're missing a check here.
The first check (with the raise RuntimeError), cbfnc and __log_callback
are both set.
Next check will enable the signal receiver if cbfnc is set. The
previous check enforces __log_callback to not be set in this case; this
is probably fine - but should have a comment of this condition.
The else: clause I'm less convinced about. If cbfnc is None,
__log_callback can also be None, which would trigger a
remove_signal_receiver() call. We should avoid that.
I'm not familiar with path email etiquette/best practices. Let me know
if/when I should send a fully updated patch.
So far, we've discussed possible solutions - so it has been fine doing
it like this now. But I think with this last round, we can go for a v2
patch.
--
kind regards,
David Sommerseth
OpenVPN Inc
_______________________________________________
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel
