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=TO7ecRHJ; dkim-atps=neutral Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.129.124]) by passt.top (Postfix) with ESMTPS id DB58F5A0265 for ; Wed, 17 Jun 2026 07:22:06 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1781673725; 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=8g76LvEUODHhvU/ZtKxlUUsej4C5vIejkq4oMHqYh5A=; b=TO7ecRHJ1KVc106c2UC5yVwkGUVGbdP4riewCL8v1K+oq7C7uTpfASs3ElLdVIC0d1TVkw qbdImKTp5mP/GETaNU1wggWmhXTKJQQskiDG1iaaYD3JMTz0OQzHEoBunXcNE1zvf4pJTe XQNYag1rQVvKc/+Q4/p73fSlzLoXRS0= Received: from mail-wr1-f70.google.com (mail-wr1-f70.google.com [209.85.221.70]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-513-yuKc9sJHMDCZeTRYGV05NQ-1; Wed, 17 Jun 2026 01:22:03 -0400 X-MC-Unique: yuKc9sJHMDCZeTRYGV05NQ-1 X-Mimecast-MFC-AGG-ID: yuKc9sJHMDCZeTRYGV05NQ_1781673722 Received: by mail-wr1-f70.google.com with SMTP id ffacd0b85a97d-46010bc0f1eso3149326f8f.3 for ; Tue, 16 Jun 2026 22:22:03 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20251104; t=1781673722; x=1782278522; 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=8g76LvEUODHhvU/ZtKxlUUsej4C5vIejkq4oMHqYh5A=; b=LIhlHgNGawK1h9+Ie7ScwIc6Yzp/jbjRxEO6d2/LcyLcHBqYIaGPhgs4PFpi4WqtgL OrjZ6GxuZSnd+P2nHMIcXaXT0s+tM/s2C/b9rk4FTyitAChG+LNLtP8ptGbWYWf00Z2y Oa85ix8Y7pGTR3CDpWy2cNKm73DUrSccPnniN+TtCgHywp8ZzLziFPS0ggQCCWpdtNUh 2Tc9ykocZQAQE91PThCKzVQO2y+bh/Nmnc9QecCu3mxRzDMC2rpotDJSGORqE5H7bC6c u/Txk1ZEkFbiIJQssgkozkgASmHWyWNXBF0W8BtL491gYXmHCfv8dCamVL3Bo1AHiPdG +3Ug== X-Gm-Message-State: AOJu0YxJzkh4XKthEfgSyRmJtFe2U7NMjaoMOPkk0PQC+b2U9btnh6m/ fxKXTVMtl4kUOb1x6iCLMB0iEPeFURoVQem7jIGrPaPbOneZAn2hnB0aZMOfqPvBxOZQyiY5uEq UGHDkzwkInZh8t+7hvH4YguTTK4/Vt+sKVQnOkAxWUUkzeWsc+0J6CKTWGshVhg== X-Gm-Gg: Acq92OFSU73fVOORFzNgUr6PlDPS869GQ4cFAbngHYtnaC/XohfH/ZfoxfxuTyXOq9L BAmDrHUOgseG1xkTlQQm2dYyl5IeV/+AJXZeiTYbJyXKdh8H06jzTqVDlZB9ihUoImammbyJlbG oqpU48G9cHhDXxNLbzEGETkor3coGN7gU9XFZNogFoew+g46IjPOP+LT4U+xXF5H+N7WDAHuBlH uPLwjBSpH/3lwLnruTzj8puSnK5x9FgDpaJe1DD244sjUVZaxMYjw0Upvux1BBgh78R+WOTj+bl 886CNf4VE/FuP+WoFXdGY5TV4FY43Y94aqR5yAY/EaBzxiZTURsm5TUA36YPWqqb1NGCHkH+yUW 1XM7MP71MRYdlORU5fOewGg== X-Received: by 2002:a05:600c:3e19:b0:490:b9c3:6c69 with SMTP id 5b1f17b1804b1-492333dd0a3mr35884295e9.30.1781673721929; Tue, 16 Jun 2026 22:22:01 -0700 (PDT) X-Received: by 2002:a05:600c:3e19:b0:490:b9c3:6c69 with SMTP id 5b1f17b1804b1-492333dd0a3mr35883755e9.30.1781673721335; Tue, 16 Jun 2026 22:22:01 -0700 (PDT) Received: from maya.myfinge.rs (ifcgrfdd.trafficplex.cloud. [2a10:fc81:a806:d6a9::1]) by smtp.gmail.com with ESMTPSA id ffacd0b85a97d-4606f2c3782sm45223046f8f.25.2026.06.16.22.22.00 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Tue, 16 Jun 2026 22:22:00 -0700 (PDT) From: Stefano Brivio To: David Gibson Subject: Re: [PATCH 2/4] flow: Include flow details with higher priority log messages Message-ID: <20260617072156.701786c1@elisabeth> In-Reply-To: References: <20260609023226.86058-1-david@gibson.dropbear.id.au> <20260609023226.86058-3-david@gibson.dropbear.id.au> <20260617010911.771f33a8@elisabeth> 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 07:22:00 +0200 (CEST) X-Mimecast-Spam-Score: 0 X-Mimecast-MFC-PROC-ID: 6_Y7LhTgkGjIANAYjYU9YFR8kGBOTfDkk_IpwV66Kyk_1781673722 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Message-ID-Hash: PAT4T6HESRXHEUWGPZZEAUV6VZ23DSME X-Message-ID-Hash: PAT4T6HESRXHEUWGPZZEAUV6VZ23DSME 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 Wed, 17 Jun 2026 12:15:54 +1000 David Gibson wrote: > On Wed, Jun 17, 2026 at 01:09:11AM +0200, Stefano Brivio wrote: > > 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()? > > That's roughly what we used to have. Right, yes, see my comment to 1/4 for why I was suggesting that an explicit flow_err_details() looked like a better interface to me. But if it's just a few corner cases I don't really have a preference. > > 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. > > .. or we could remove the separate print of the details, knowing it > was included in flow_err()? Or maybe I don't quite grasp the case > you're describing. Yes, that's the case I was describing. I was thinking of a function failing to do something with a flow, and then reporting further details. But I guess it's not very likely to happen anyway. > > Not a strong preference from my side, I also see the value of keeping > > this terse like the current patch does. > > Terse is nice, but for me the bigger advantage is it's much harder to > forget to include relevant details in an error/warning message. Ah, sure, fair point. > Part > of the context of this is debugging > https://github.com/podman-container-tools/podman/issues/23739 where we > were seeing an error message that was somewhat useful, but would have > been much more useful if we knew the details of the flow it applied > to. Right, I see now. -- Stefano