Re: [Openvpn-devel] [PATCH] Fix StatusChangeCallback so it works without a LogCallback

2023-09-04 Thread Jeremy Fleischman
> I have a feeling we're missing a check here.

Got it! You're right, I've added that in the v2 version of this patch
(see below).

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.DBu

Re: [Openvpn-devel] [PATCH] Fix StatusChangeCallback so it works without a LogCallback

2023-09-04 Thread David Sommerseth

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


Re: [Openvpn-devel] [PATCH] Fix StatusChangeCallback so it works without a LogCallback

2023-09-04 Thread Jeremy Fleischman
>  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'm not familiar with path email etiquette/best practices. Let me know
if/when I should send a fully updated patch.


___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel


Re: [Openvpn-devel] [PATCH] Fix StatusChangeCallback so it works without a LogCallback

2023-09-04 Thread David Sommerseth

[re-sent to ML]

On 04/09/2023 19:51, Jeremy Fleischman wrote:

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


Oops, this code unconditionally removes the callback, even if there
isn't currently a callback. The code below in StatusChangeCallback
first checks if there is currently a callback registered before removing
it. If `self.__dbuscon.remove_signal_receiver` is resilient to getting
passed None values for callbacks, then I suppose would could skip the
check and just unconditionally remove the callback.
Let me know what's best.


If you look at my proposal; in the LogCallback() method, I have this check:

if cbfnc is not None and self.__log_callback is None:

while you only do:

if cbfnc is not None:

I believe you need to check both (more below on that).  And there is a 
similar logic check when disabling the callback:


elif cbfnc is None and self.__log_callback is not None:

I believe this is the trap you fell into.  Your new patch just do an "else:"

The additional final check of setting a new callback without disabling 
the previous one can avoid some additional trouble.  IIRC, the Python 
D-Bus API will actually call multiple callbacks if add_signal_receiver() 
is called more times with different callback methods.  To support this, 
the LogCallback() and StatusChangeCallback() methods would need to track 
each callback function independently.   I did not consider this a needed 
feature just yet; hence the additional final check:


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.



Otherwise, I like what you did to __set_log_forward().  That makes sense!



--
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] Fix StatusChangeCallback so it works without a LogCallback

2023-09-04 Thread Jeremy Fleischman
> 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

Oops, this code unconditionally removes the callback, even if there
isn't currently a callback. The code below in StatusChangeCallback
first checks if there is currently a callback registered before removing
it. If `self.__dbuscon.remove_signal_receiver` is resilient to getting
passed None values for callbacks, then I suppose would could skip the
check and just unconditionally remove the callback.
Let me know what's best.

>
> +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


Re: [Openvpn-devel] [PATCH] Fix StatusChangeCallback so it works without a LogCallback

2023-09-04 Thread Jeremy Fleischman
Hey David,

On Sun, Sep 3, 2023 at 3:56 PM David Sommerseth
 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


[Openvpn-devel] [XS] Change in openvpn[release/2.6]: Support for long INFO/INFO_PRE messages

2023-09-04 Thread stipa (Code Review)
Attention is currently required from: flichtenheld.

stipa has posted comments on this change. ( 
http://gerrit.openvpn.net/c/openvpn/+/327?usp=email )

Change subject: Support for long INFO/INFO_PRE messages
..


Patch Set 2:

(1 comment)

File src/openvpn/push.c:

http://gerrit.openvpn.net/c/openvpn/+/327/comment/a70eded3_8b1ba798 :
PS1, Line 246: struct buffer out = alloc_buf_gc(1 + 7 + 1 + BLEN(&buf) 
+ 1, &gc);
> "1 + 7 + 1" -> strlen(">INFOMSG:") […]
Acknowledged



--
To view, visit http://gerrit.openvpn.net/c/openvpn/+/327?usp=email
To unsubscribe, or for help writing mail filters, visit 
http://gerrit.openvpn.net/settings

Gerrit-Project: openvpn
Gerrit-Branch: release/2.6
Gerrit-Change-Id: I2139b62117ba69d643b585d2610e8aef15f71d3e
Gerrit-Change-Number: 327
Gerrit-PatchSet: 2
Gerrit-Owner: stipa 
Gerrit-Reviewer: flichtenheld 
Gerrit-CC: openvpn-devel 
Gerrit-Attention: flichtenheld 
Gerrit-Comment-Date: Mon, 04 Sep 2023 15:44:36 +
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: flichtenheld 
Gerrit-MessageType: comment
___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel


[Openvpn-devel] [XS] Change in openvpn[release/2.6]: Support for long INFO/INFO_PRE messages

2023-09-04 Thread stipa (Code Review)
Attention is currently required from: flichtenheld, stipa.

Hello flichtenheld,

I'd like you to reexamine a change. Please visit

http://gerrit.openvpn.net/c/openvpn/+/327?usp=email

to look at the new patch set (#2).

The following approvals got outdated and were removed:
Code-Review+1 by flichtenheld


Change subject: Support for long INFO/INFO_PRE messages
..

Support for long INFO/INFO_PRE messages

Current hardcoded limit is not defined anywhere and
the server (CloudConnexa) sends the longer string in some cases.

Change-Id: I2139b62117ba69d643b585d2610e8aef15f71d3e
Signed-off-by: Lev Stipakov 
---
M src/openvpn/push.c
1 file changed, 4 insertions(+), 2 deletions(-)


  git pull ssh://gerrit.openvpn.net:29418/openvpn refs/changes/27/327/2

diff --git a/src/openvpn/push.c b/src/openvpn/push.c
index d468211..722bfce 100644
--- a/src/openvpn/push.c
+++ b/src/openvpn/push.c
@@ -42,6 +42,8 @@

 static char push_reply_cmd[] = "PUSH_REPLY";

+#define INFOMSG_MAN ">INFOMSG:"
+
 /*
  * Auth username/password
  *
@@ -243,8 +245,8 @@
  * We use >INFOMSG here instead of plain >INFO since INFO is used to
  * for management greeting and we don't want to confuse the client
  */
-struct buffer out = alloc_buf_gc(256, &gc);
-buf_printf(&out, ">%s:%s", "INFOMSG", m);
+struct buffer out = alloc_buf_gc(strlen(INFOMSG_MAN) + BLEN(&buf), 
&gc);
+buf_printf(&out, "%s%s", INFOMSG_MAN, m);
 management_notify_generic(management, BSTR(&out));

 gc_free(&gc);

--
To view, visit http://gerrit.openvpn.net/c/openvpn/+/327?usp=email
To unsubscribe, or for help writing mail filters, visit 
http://gerrit.openvpn.net/settings

Gerrit-Project: openvpn
Gerrit-Branch: release/2.6
Gerrit-Change-Id: I2139b62117ba69d643b585d2610e8aef15f71d3e
Gerrit-Change-Number: 327
Gerrit-PatchSet: 2
Gerrit-Owner: stipa 
Gerrit-Reviewer: flichtenheld 
Gerrit-CC: openvpn-devel 
Gerrit-Attention: flichtenheld 
Gerrit-Attention: stipa 
Gerrit-MessageType: newpatchset
___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel


[Openvpn-devel] [XS] Change in openvpn[release/2.6]: Support for long INFO/INFO_PRE messages

2023-09-04 Thread flichtenheld (Code Review)
Attention is currently required from: stipa.

flichtenheld has posted comments on this change. ( 
http://gerrit.openvpn.net/c/openvpn/+/327?usp=email )

Change subject: Support for long INFO/INFO_PRE messages
..


Patch Set 1: Code-Review+1

(3 comments)

Commit Message:

http://gerrit.openvpn.net/c/openvpn/+/327/comment/90da86d9_5dc75b60 :
PS1, Line 10: the server (CloudConnexa) sends the longer string in some cases.
"sends the longer string" -> "sends longer messages"


Patchset:

PS1:
looks technically okay


File src/openvpn/push.c:

http://gerrit.openvpn.net/c/openvpn/+/327/comment/cf2407e8_9d3632ee :
PS1, Line 246: struct buffer out = alloc_buf_gc(1 + 7 + 1 + BLEN(&buf) 
+ 1, &gc);
"1 + 7 + 1" -> strlen(">INFOMSG:")
might even move the ">INFOMSG:" into a preprocessor define and then reuse it.



--
To view, visit http://gerrit.openvpn.net/c/openvpn/+/327?usp=email
To unsubscribe, or for help writing mail filters, visit 
http://gerrit.openvpn.net/settings

Gerrit-Project: openvpn
Gerrit-Branch: release/2.6
Gerrit-Change-Id: I2139b62117ba69d643b585d2610e8aef15f71d3e
Gerrit-Change-Number: 327
Gerrit-PatchSet: 1
Gerrit-Owner: stipa 
Gerrit-Reviewer: flichtenheld 
Gerrit-CC: openvpn-devel 
Gerrit-Attention: stipa 
Gerrit-Comment-Date: Mon, 04 Sep 2023 14:39:24 +
Gerrit-HasComments: Yes
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment
___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel


[Openvpn-devel] [XS] Change in openvpn[release/2.6]: Support for long INFO/INFO_PRE messages

2023-09-04 Thread stipa (Code Review)
Attention is currently required from: flichtenheld.

Hello flichtenheld,

I'd like you to do a code review.
Please visit

http://gerrit.openvpn.net/c/openvpn/+/327?usp=email

to review the following change.


Change subject: Support for long INFO/INFO_PRE messages
..

Support for long INFO/INFO_PRE messages

Current hardcoded limit is not defined anywhere and
the server (CloudConnexa) sends the longer string in some cases.

Change-Id: I2139b62117ba69d643b585d2610e8aef15f71d3e
Signed-off-by: Lev Stipakov 
---
M src/openvpn/push.c
1 file changed, 1 insertion(+), 1 deletion(-)



  git pull ssh://gerrit.openvpn.net:29418/openvpn refs/changes/27/327/1

diff --git a/src/openvpn/push.c b/src/openvpn/push.c
index d468211..663cfbd 100644
--- a/src/openvpn/push.c
+++ b/src/openvpn/push.c
@@ -243,7 +243,7 @@
  * We use >INFOMSG here instead of plain >INFO since INFO is used to
  * for management greeting and we don't want to confuse the client
  */
-struct buffer out = alloc_buf_gc(256, &gc);
+struct buffer out = alloc_buf_gc(1 + 7 + 1 + BLEN(&buf) + 1, &gc);
 buf_printf(&out, ">%s:%s", "INFOMSG", m);
 management_notify_generic(management, BSTR(&out));


--
To view, visit http://gerrit.openvpn.net/c/openvpn/+/327?usp=email
To unsubscribe, or for help writing mail filters, visit 
http://gerrit.openvpn.net/settings

Gerrit-Project: openvpn
Gerrit-Branch: release/2.6
Gerrit-Change-Id: I2139b62117ba69d643b585d2610e8aef15f71d3e
Gerrit-Change-Number: 327
Gerrit-PatchSet: 1
Gerrit-Owner: stipa 
Gerrit-Reviewer: flichtenheld 
Gerrit-CC: openvpn-devel 
Gerrit-Attention: flichtenheld 
Gerrit-MessageType: newchange
___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel