diff mbox series

[Openvpn-devel,v1] dco: avoid printing mi prefix on float

Message ID 20250902115954.29021-1-gert@greenie.muc.de
State Accepted
Headers
Series [Openvpn-devel,v1] dco: avoid printing mi prefix on float |

Commit Message

Gert Doering Sept. 2, 2025, 11:59 a.m. UTC 
  From: Gianmarco De Gregori <gianmarco@mandelbit.com>

On float, a new prefix is generated which leads
to a mismatch between the current and previous
peer-id being logged. To avoid this, the
M_NOIPREFIX flag is now used along with msglevel.

Change-Id: I84a73d625c79d6a6a19122e48c91960dbe01ec49
Signed-off-by: Gianmarco De Gregori <gianmarco@mandelbit.com>
Acked-by: Gert Doering <gert@greenie.muc.de>
---

This change was reviewed on Gerrit and approved by at least one
developer. I request to merge it to master.

Gerrit URL: https://gerrit.openvpn.net/c/openvpn/+/1158
This mail reflects revision 1 of this Change.

Acked-by according to Gerrit (reflected above):
Gert Doering <gert@greenie.muc.de>

Comments

Gert Doering Sept. 2, 2025, 4:51 p.m. UTC | #1 
Tested on Linux/DCO (my "float victim"), and it now is much less confusing
- there's clear "this is for a given peer" messages that include a prefix,
and there's low-level DCO informational messages which often come in
asynchronously, not related to the current mi peer.  With M_NOIPREFIX
these never get a prefix - and where relevant, they print the peer-id
anyway

+    msg(D_DCO_DEBUG | M_NOIPREFIX, "%s: parsing message for peer %u...", __func__, peer_id);

(and D_DCO_DEBUG is "verb 7", so there's enough messages that link peer-id
to a given client instance)

I'm not sure if we shouldn't do a bit larger rehaul of this, like always
include M_NOIPREFIX in the very definition of D_DCO_DEBUG (and all other
D_* levels that are similar in nature) - the whole "prefix" thing has been
there from day one, and we haven't really spent much thought on it (yet) -
but that might be a 2.8 topic, do what Selva has wanted to do for a long
time, and go with a fine comb over all the msg() and --verb levels...

The sig.c change is one of those places - ideally, everybody who 
raises a signal has the proper prefix beforehand (or resets it) - but
for "DCO has an error to raise" this is not really straightforward.


I have changed the commit message from "on float" to "on debug messages",
as this affected peer stats just as well - basically, any asynchronous
DCO event that wants to print infos.


Yet again, mail-archive.org is trying to annoy me - claiming there is
no such mail.  So, linking to sourceforge.net archive, again.

Your patch has been applied to the master branch.

commit 9e9ba09adbe08e93d91d8340efe63bae978f5f34
Author: Gianmarco De Gregori
Date:   Tue Sep 2 13:59:49 2025 +0200

     dco: avoid printing mi prefix on float

     Signed-off-by: Gianmarco De Gregori <gianmarco@mandelbit.com>
     Acked-by: Gert Doering <gert@greenie.muc.de>
     Message-Id: <20250902115954.29021-1-gert@greenie.muc.de>
     URL: https://sourceforge.net/p/openvpn/mailman/message/59228149/
     Signed-off-by: Gert Doering <gert@greenie.muc.de>


--
kind regards,

Gert Doering
diff mbox series

Patch

diff --git a/src/openvpn/dco_linux.c b/src/openvpn/dco_linux.c
index a3907fe..6115d51 100644
--- a/src/openvpn/dco_linux.c
+++ b/src/openvpn/dco_linux.c
@@ -548,7 +548,7 @@ 
 int
 dco_del_peer(dco_context_t *dco, unsigned int peerid)
 {
-    msg(D_DCO_DEBUG, "%s: peer-id %d", __func__, peerid);
+    msg(D_DCO_DEBUG | M_NOIPREFIX, "%s: peer-id %d", __func__, peerid);
 
     struct nl_msg *nl_msg = ovpn_dco_nlmsg_create(dco, OVPN_CMD_PEER_DEL);
     if (!nl_msg)
@@ -868,7 +868,7 @@ 
     uint32_t peer_id = nla_get_u32(tb_peer[OVPN_A_PEER_ID]);
     struct context_2 *c2;
 
-    msg(D_DCO_DEBUG, "%s: parsing message for peer %u...", __func__, peer_id);
+    msg(D_DCO_DEBUG | M_NOIPREFIX, "%s: parsing message for peer %u...", __func__, peer_id);
 
     if (dco->ifmode == OVPN_MODE_P2P)
     {
@@ -890,7 +890,7 @@ 
         struct multi_instance *mi = dco->c->multi->instances[peer_id];
         if (!mi)
         {
-            msg(M_WARN, "%s: received data for a non-existing peer %u", __func__, peer_id);
+            msg(M_WARN | M_NOIPREFIX, "%s: received data for a non-existing peer %u", __func__, peer_id);
             return NL_SKIP;
         }
 
@@ -934,32 +934,32 @@ 
 
     if (!attrs[OVPN_A_PEER])
     {
-        msg(D_DCO, "ovpn-dco: no peer in PEER_DEL_NTF message");
+        msg(D_DCO | M_NOIPREFIX, "ovpn-dco: no peer in PEER_DEL_NTF message");
         return NL_STOP;
     }
 
     struct nlattr *dp_attrs[OVPN_A_PEER_MAX + 1];
     if (nla_parse_nested(dp_attrs, OVPN_A_PEER_MAX, attrs[OVPN_A_PEER], NULL))
     {
-        msg(D_DCO, "ovpn-dco: can't parse peer in PEER_DEL_NTF messsage");
+        msg(D_DCO | M_NOIPREFIX, "ovpn-dco: can't parse peer in PEER_DEL_NTF messsage");
         return NL_STOP;
     }
 
     if (!dp_attrs[OVPN_A_PEER_DEL_REASON])
     {
-        msg(D_DCO, "ovpn-dco: no reason in PEER_DEL_NTF message");
+        msg(D_DCO | M_NOIPREFIX, "ovpn-dco: no reason in PEER_DEL_NTF message");
         return NL_STOP;
     }
     if (!dp_attrs[OVPN_A_PEER_ID])
     {
-        msg(D_DCO, "ovpn-dco: no peer-id in PEER_DEL_NTF message");
+        msg(D_DCO | M_NOIPREFIX, "ovpn-dco: no peer-id in PEER_DEL_NTF message");
         return NL_STOP;
     }
 
     int reason = nla_get_u32(dp_attrs[OVPN_A_PEER_DEL_REASON]);
     unsigned int peerid = nla_get_u32(dp_attrs[OVPN_A_PEER_ID]);
 
-    msg(D_DCO_DEBUG, "ovpn-dco: received CMD_PEER_DEL_NTF, ifindex: %d, peer-id %u, reason: %d",
+    msg(D_DCO_DEBUG | M_NOIPREFIX, "ovpn-dco: received CMD_PEER_DEL_NTF, ifindex: %d, peer-id %u, reason: %d",
         dco->ifindex, peerid, reason);
     dco->dco_message_peer_id = peerid;
     dco->dco_del_peer_reason = reason;
@@ -1065,7 +1065,7 @@ 
     struct nlmsghdr *nlh = nlmsg_hdr(msg);
     struct genlmsghdr *gnlh = genlmsg_hdr(nlh);
 
-    msg(D_DCO_DEBUG, "ovpn-dco: received netlink message type=%u cmd=%u flags=%#.4x",
+    msg(D_DCO_DEBUG | M_NOIPREFIX, "ovpn-dco: received netlink message type=%u cmd=%u flags=%#.4x",
         nlh->nlmsg_type, gnlh->cmd, nlh->nlmsg_flags);
 
     /* if we get a message from the NLCTRL family, it means
@@ -1148,7 +1148,7 @@ 
         return 0;
     }
 
-    msg(D_DCO_DEBUG, "%s: peer-id %d", __func__, peer_id);
+    msg(D_DCO_DEBUG | M_NOIPREFIX, "%s: peer-id %d", __func__, peer_id);
 
     struct nl_msg *nl_msg = ovpn_dco_nlmsg_create(dco, OVPN_CMD_PEER_GET);
     struct nlattr *attr = nla_nest_start(nl_msg, OVPN_A_PEER);
diff --git a/src/openvpn/sig.c b/src/openvpn/sig.c
index 5f5a808..ecea635 100644
--- a/src/openvpn/sig.c
+++ b/src/openvpn/sig.c
@@ -239,7 +239,7 @@ 
         {
             si->source = SIG_SOURCE_CONNECTION_FAILED;
         }
-        msg(D_SIGNAL_DEBUG, "register signal: %s (%s)", signal_name(signum, true), signal_text);
+        msg(D_SIGNAL_DEBUG | M_NOIPREFIX, "register signal: %s (%s)", signal_name(signum, true), signal_text);
     }
     else
     {