From mboxrd@z Thu Jan 1 00:00:00 1970 Authentication-Results: passt.top; dmarc=pass (p=quarantine dis=none) header.from=redhat.com Authentication-Results: passt.top; dkim=pass (1024-bit key; unprotected) header.d=redhat.com header.i=@redhat.com header.a=rsa-sha256 header.s=mimecast20190719 header.b=YPyCl76h; dkim-atps=neutral Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.133.124]) by passt.top (Postfix) with ESMTPS id 04EE25A0262 for ; Wed, 17 Jun 2026 01:09:16 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1781651355; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=OpAs/itsigiFMnobLR91pUSuf8WQHtQfmbRctMG3xrI=; b=YPyCl76hYjPZ8Y/aNaroXI03gSlPORK3FOVkyuZ2eP1tV+5SFONpoDasGXr80GK4hoSKaW 5vUdVEVMYZDaI3i1dh5G9xOkZ3hzgqC7jEYJnBP/iNuWyXx7M1tg56+geJxLUIDNMLmbTk SnI1D3poEkHo0CbdQEm1wOsuIO2YNJ0= Received: from mail-wr1-f71.google.com (mail-wr1-f71.google.com [209.85.221.71]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-322-YlgyVsA1OnC5hatPe08eaQ-1; Tue, 16 Jun 2026 19:09:14 -0400 X-MC-Unique: YlgyVsA1OnC5hatPe08eaQ-1 X-Mimecast-MFC-AGG-ID: YlgyVsA1OnC5hatPe08eaQ_1781651353 Received: by mail-wr1-f71.google.com with SMTP id ffacd0b85a97d-45eec2ba533so3250035f8f.3 for ; Tue, 16 Jun 2026 16:09:14 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20251104; t=1781651353; x=1782256153; h=date:content-transfer-encoding:mime-version:organization:references :in-reply-to:message-id:subject:cc:to:from:x-gm-gg :x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; bh=OpAs/itsigiFMnobLR91pUSuf8WQHtQfmbRctMG3xrI=; b=sqKeAD3m9qQbb0qkSfxpoh+ZuHmqI1uvL5FdKiftlCwE8fjzlEdfKvJw26C4GwFwME pOD7R0XP3/OostpAsMEgKBNsPEhAhS0LS7TglTYdYUNhVbLW+Jl41FYFf3oXG0I0Z/9Q hhkjkvvcRs5tdr3rip5I1ZN1ER53+CgaOW0U7O5dpB7jWfKuTk8Y2SVfe6UiNqyd2ru7 EweWFasD5zyiehVq2MF7d8V2KyP+GerLJDNZnQ6Z/5uK/WLxQeGIL/ZkTo6FyLesUMvh rbwiPhCbI01IjrV4JQ42jva5wE6DUF1MAmXIe09fBw9tZyWBmyDHAe8HYOBR6wrce9Az 4VzA== X-Gm-Message-State: AOJu0YwOTkFKrdS/k/D3waY5CmgVXavBhnfKdDOh20ZQ4ed2YfTS7uaQ XfnHzxfJL9rsUfMPMY1senRwojD/d20vGRLidPsZ56n9Wn1axgJOOcvSOcaXevqPLHj6P9GockQ NS3tEiUIhZ2KB9q6x/SbkO9O7oYxNQexDgHs8ZDx61Go5tQqu2eU+gjIgNRS5Qg== X-Gm-Gg: AfdE7cnVoIMdcOFerCPAfFRew7aLehv1KrMx67xrZVjyIAhojCcC0wkQ3WJFXtdhZPT IESKJQ2RVR881OO6nCWN5X3DCcVAuCXtqQWsrZ5bYMyEa5r/KmekFhmn2WSeJAw8EdcEXkhD2Eg eqZqXSAMP+CLm9QrHLgkeSRx+8Jdk9irgsHJJ6yJ/c1Op47X16WPvup1BB0ffo3LbAMvbXI+Jnj Z1KjNjVTgT+XYszllF6v5rEO7nAKpalOda/3oyIYpR3FuMOX+0938iRBbgoRTfQe/b8hW0/BA1V Z1ByZKI7J5yaTb7JfDphy+laqy24vBDUgYSQXiHwmgrjqr765NFX5Ya1oGiiMb+h/29DJgYPMIZ eeiECXzTqr/nBq976S/0XWAWNWcYgbBXo76ofLXmIOyVXX7bKqQ== X-Received: by 2002:a5d:4a91:0:b0:45e:e50e:3bc with SMTP id ffacd0b85a97d-4623788f72amr1816413f8f.29.1781651353335; Tue, 16 Jun 2026 16:09:13 -0700 (PDT) X-Received: by 2002:a5d:4a91:0:b0:45e:e50e:3bc with SMTP id ffacd0b85a97d-4623788f72amr1816383f8f.29.1781651352772; Tue, 16 Jun 2026 16:09:12 -0700 (PDT) Received: from maya.myfinge.rs (ifcgrfdd.trafficplex.cloud. [176.103.220.4]) by smtp.gmail.com with ESMTPSA id ffacd0b85a97d-4606f2b0d4fsm52495014f8f.24.2026.06.16.16.09.12 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Tue, 16 Jun 2026 16:09:12 -0700 (PDT) From: Stefano Brivio To: David Gibson Subject: Re: [PATCH 2/4] flow: Include flow details with higher priority log messages Message-ID: <20260617010911.771f33a8@elisabeth> In-Reply-To: <20260609023226.86058-3-david@gibson.dropbear.id.au> References: <20260609023226.86058-1-david@gibson.dropbear.id.au> <20260609023226.86058-3-david@gibson.dropbear.id.au> Organization: Red Hat X-Mailer: Claws Mail 4.2.0 (GTK 3.24.49; x86_64-pc-linux-gnu) MIME-Version: 1.0 Date: Wed, 17 Jun 2026 01:09:11 +0200 (CEST) X-Mimecast-Spam-Score: 0 X-Mimecast-MFC-PROC-ID: hlQgk3bma221btV6byNB59TtCQwtN8ODNOrrkDT_adE_1781651353 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Message-ID-Hash: ICSST7T43MZLYK3XEDOUVAEL7WICYTFM X-Message-ID-Hash: ICSST7T43MZLYK3XEDOUVAEL7WICYTFM X-MailFrom: sbrivio@redhat.com X-Mailman-Rule-Misses: dmarc-mitigation; no-senders; approved; emergency; loop; banned-address; member-moderation; nonmember-moderation; administrivia; implicit-dest; max-recipients; max-size; news-moderation; no-subject; digests; suspicious-header CC: passt-dev@passt.top X-Mailman-Version: 3.3.8 Precedence: list List-Id: Development discussion and patches for passt Archived-At: Archived-At: List-Archive: List-Archive: List-Help: List-Owner: List-Post: List-Subscribe: List-Unsubscribe: On Tue, 9 Jun 2026 12:32:24 +1000 David Gibson wrote: > Currently flow_log() and related functions / macros have a 'details' > parameter which indicates whether to add extra messages with details of the > flow's addresses. This is still a bit awkward to invoke, and only used in > a few places. Change the logic, to automatically include the details if > and only if the log priority is greater than LOG_DEBUG. > > Rationale: > > If at debug log level, there are already a bunch of debug messages tracking > the flow life cycle, which include those details (we make sure to retain > those). It's usually pretty easy to cross reference a specific flow debug > message with the flow's history including the details. > > If at higher log level, and we generate a flow-connected error or warning > we don't have those life cycle messages. So, just giving the flow index > doesn't really tell you anything about which flow tripped the error. > Adding the address details make the error message significantly more > useful. > > Signed-off-by: David Gibson > --- > flow.c | 2 +- > flow.h | 22 +++++++++++----------- > udp.c | 5 ++--- > 3 files changed, 14 insertions(+), 15 deletions(-) > > diff --git a/flow.c b/flow.c > index 6cf3905a..dd92bad7 100644 > --- a/flow.c > +++ b/flow.c > @@ -550,7 +550,7 @@ norule: > /* This shouldn't happen, because if there's no rule for it we should > * have no listening socket that would let us get here > */ > - flow_log(flow, LOG_DEBUG, false, true, "Missing forward rule"); > + flow_dbg(flow, "Missing forward rule"); > > nofwd: > flow_err(flow, "No rules to forward %s %s [%s]:%hu -> [%s]:%hu", > diff --git a/flow.h b/flow.h > index d168a35a..e055defb 100644 > --- a/flow.h > +++ b/flow.h > @@ -283,19 +283,19 @@ int flow_migrate_target(struct ctx *c, const struct migrate_stage *stage, > void flow_log__(const struct flow_common *f, int pri, bool perror, bool details, > enum flow_state state, const char *fmt, ...); > > -#define flow_log_(f_, pri_, perror_, details_, ...) \ > - flow_log__((f_), (pri_), (perror_), (details_), (f_)->state, \ > - __VA_ARGS__) > +#define flow_log_(f_, pri_, perror_, ...) \ > + flow_log__((f_), (pri_), (perror_), (pri_) > LOG_DEBUG, \ On one hand, this looks quite practical, and we need the effects of this change anyway. On the other hand, it's a bit sneaky to do this implicitly. I wonder: > + (f_)->state, __VA_ARGS__) > > -#define flow_log(flow_, pri_, perror_, details_, ...) \ > - flow_log_(&(flow_)->f, (pri_), (perror_), (details_), __VA_ARGS__) > +#define flow_log(flow_, pri_, perror_, ...) \ > + flow_log_(&(flow_)->f, (pri_), (perror_), __VA_ARGS__) > > #define flow_dbg(flow_, ...) \ > - flow_log((flow_), LOG_DEBUG, false, false, __VA_ARGS__) > + flow_log((flow_), LOG_DEBUG, false, __VA_ARGS__) > #define flow_warn(flow_, ...) \ > - flow_log((flow_), LOG_WARNING, false, false, __VA_ARGS__) > + flow_log((flow_), LOG_WARNING, false, __VA_ARGS__) > #define flow_err(flow_, ...) \ > - flow_log((flow_), LOG_ERR, false, false, __VA_ARGS__) > + flow_log((flow_), LOG_ERR, false, __VA_ARGS__) > #define flow_trace(flow_, ...) \ > do { \ > if (log_trace) \ > @@ -303,11 +303,11 @@ void flow_log__(const struct flow_common *f, int pri, bool perror, bool details, > } while (0) > > #define flow_dbg_perror(flow_, ...) \ > - flow_log((flow_), LOG_DEBUG, true, false, __VA_ARGS__) > + flow_log((flow_), LOG_DEBUG, true, __VA_ARGS__) > #define flow_warn_perror(flow_, ...) \ > - flow_log((flow_), LOG_WARNING, true, false, __VA_ARGS__) > + flow_log((flow_), LOG_WARNING, true, __VA_ARGS__) > #define flow_perror(flow_, ...) \ > - flow_log((flow_), LOG_ERR, true, false, __VA_ARGS__) > + flow_log((flow_), LOG_ERR, true, __VA_ARGS__) > > #define flow_dbg_ratelimit(flow_, now_, ...) \ > logmsg_ratelimit(flow_dbg, debug, (now_), (flow_), __VA_ARGS__) > diff --git a/udp.c b/udp.c > index f29ca3da..caeedf8f 100644 > --- a/udp.c > +++ b/udp.c > @@ -943,8 +943,7 @@ void udp_sock_handler(const struct ctx *c, union epoll_ref ref, > > if (events & EPOLLERR) { > if (udp_sock_errs(c, ref.fd, ref.flowside, PIF_NONE, 0) < 0) { > - flow_log(uflow, LOG_ERR, false, true, > - "Unrecoverable error on flow socket"); > + flow_err(uflow, "Unrecoverable error on flow socket"); ...what if this, and... > goto fail; > } > } > @@ -975,7 +974,7 @@ void udp_sock_handler(const struct ctx *c, union epoll_ref ref, > udp_buf_sock_to_tap(c, s, n, tosidx); > } > } else { > - flow_log(uflow, LOG_ERR, false, true, > + flow_err(uflow, ...this would both become flow_err_details()? There might be future cases where we already print flow details separately, even above LOG_DEBUG, and in those cases we could keep calling flow_err() without the details. Not a strong preference from my side, I also see the value of keeping this terse like the current patch does. > "No support for forwarding UDP from %s to %s", > pif_name(pif_at_sidx(ref.flowside)), > pif_name(topif)); -- Stefano