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=fp/D4E1+; 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 CE6895A0265 for ; Thu, 17 Sep 2026 21:08:22 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1789672101; 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=EKPkgJQmpZMHhn0h0aQBFfZoSVgtZYLYsZMd8yo9Xxg=; b=fp/D4E1+i+CFadgjeXmyoybtUGrvQc4hQYnO+fg3HVW3mURzV9ZIaRyP85X7Cn0BKddAmw WCYK85NxnC+wNRr78+HgwpU5erBYhDKUNSXWU1CVZdkc2ibtK+rPLuToKmlmdtiBICLLfV 0vF8lfmGNTNigmM/Fb+zpeHR1TeJ+OI= Received: from mail-wm1-f69.google.com (mail-wm1-f69.google.com [209.85.128.69]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-554-R2Jj5qiIMX2E0St6DUt2yQ-1; Thu, 17 Sep 2026 15:08:17 -0400 X-MC-Unique: R2Jj5qiIMX2E0St6DUt2yQ-1 X-Mimecast-MFC-AGG-ID: R2Jj5qiIMX2E0St6DUt2yQ_1789672096 Received: by mail-wm1-f69.google.com with SMTP id 5b1f17b1804b1-49e745d5dabso12227205e9.0 for ; Thu, 17 Sep 2026 12:08:16 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20260707; t=1789672096; x=1790276896; h=date:content-transfer-encoding:content-type: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:content-type; bh=EKPkgJQmpZMHhn0h0aQBFfZoSVgtZYLYsZMd8yo9Xxg=; b=g1gt3zok7q4yFuVLdkyF7CKsuma8DrhOj75aVuzv+INFxokWoNyxmPmdFd1mV1NNEa uZzqDM9NzQPwttbxaL6584odVRa73SEpw0bjDwJi3pjT5hjgkKCnhEP+Ijwb0uI1PC0l YVlHKOeKuYYNohpT/gmMAkt0II6NUcS689h4H6qvHkP6bgvb9MZumzreN9mD7QiOOjDe G9s/7l8K8kzsducG1fTQRB3mf8fUDHaJm8AgY6JP8+90VIlMniv8D26geshCCSQniPP+ y+soEW57Oi04CedNmANFO607Fmjy9+sEp49t/bhuSV4CQgkyAuFucsIl7s8af4BfsYch H0FA== X-Gm-Message-State: AFuF++lj/618Hcb0wMD79OdiTLS7zFetf0Z5+a3ElSFLqwkxW32/fDpJ HWjgrDNisCtN25R+7RMfdtKQVP/jZmxmoC0AK987eFf1MLouP3aDdkmaXcXWmBPSfQ1q7HN44A5 wIkI8dOjzYQLNPYGHm9GPlHkWCxvUdNxZR8vGUEDK32dWtsZXx2QRYQ== X-Gm-Gg: AYBFou0ioiLsXATRQS1lt4dGGgJ497dL30ml+yBvrEjE+2+YMHWC60fVORMn5RdF8rK gPVBVVq+NZPTTo/MSFNy/MTloT6c+BVzwWp+p6fWEP3A/ic+f1f5dcrGllQ5IBZGcAb/LfZ6fVV 1iFuk9s+1p7dIhSJkvNfTzFIjv/LZUBdbXcXkw/E2osTvwYsD4HQs5ebJC04x5Z+zYPn2Kr6ROT 9/u4HI3cbkjsVKEuIuUh5Pwww6gW3Xki9e8WQMXWq8mdGBJv6Fc4IZdsoqnzLtA0VcSufFYKd7B 9Hu5+fBgVUBWy7ExZdwPEzYWqQnWHQC+UFlzMc6CgebuNz2iiZPi+N5vcvVqyrbkVkwinbiebHj UcuxZciTgViq1PTR6Zv3cPpHNeSgQ X-Received: by 2002:a05:600c:468c:b0:49d:2450:68ac with SMTP id 5b1f17b1804b1-49eb72f3c22mr116831355e9.5.1789672095706; Thu, 17 Sep 2026 12:08:15 -0700 (PDT) X-Received: by 2002:a05:600c:468c:b0:49d:2450:68ac with SMTP id 5b1f17b1804b1-49eb72f3c22mr116831005e9.5.1789672095044; Thu, 17 Sep 2026 12:08:15 -0700 (PDT) Received: from maya.myfinge.rs (ifcgrfdd.trafficplex.cloud. [176.103.220.4]) by smtp.gmail.com with ESMTPSA id 5b1f17b1804b1-49fc52115d9sm587735e9.2.2026.09.17.12.08.14 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Thu, 17 Sep 2026 12:08:14 -0700 (PDT) From: Stefano Brivio To: Martin Schitter Subject: Re: [PATCH v2 1/3] Fix handling of netlink multipart route dumps. Message-ID: <20260917210812.44e10453@elisabeth> In-Reply-To: <20260908181631.537802-2-ms+git@mur.at> References: <20260908181631.537802-1-ms+git@mur.at> <20260908181631.537802-2-ms+git@mur.at> Organization: Red Hat X-Mailer: Claws Mail 4.2.0 (GTK 3.24.49; x86_64-pc-linux-gnu) MIME-Version: 1.0 Date: Thu, 17 Sep 2026 21:08:13 +0200 (CEST) X-Mimecast-Spam-Score: 0 X-Mimecast-MFC-PROC-ID: UvCPYjISK1XtXjw33rxb8g60DI-jHF7IRu8qQlBFVgE_1789672096 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Message-ID-Hash: IEMTHKMB4PQY6G7H5ZWZADICGUKEW6ZX X-Message-ID-Hash: IEMTHKMB4PQY6G7H5ZWZADICGUKEW6ZX 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: Here's my review, sorry for the delay. I have mostly coding style remarks on this patch, and more substantial comments in the following patches. Conceptually, though, this patch entirely makes sense to me. On Tue, 8 Sep 2026 18:13:12 +0000 Martin Schitter wrote: > This patch adds a `nl_collect` function to store all datagram > chunks of a multipart route dump in one single interim buffer > for further processing and dependency resolution. Here, and in following patches: missing Signed-off-by: tag (see CONTRIBUTING.md). > --- > netlink.c | 79 +++++++++++++++++++++++++++++++++++++++---------------- > 1 file changed, 56 insertions(+), 23 deletions(-) > > diff --git a/netlink.c b/netlink.c > index 225b666..650a6fd 100644 > --- a/netlink.c > +++ b/netlink.c > @@ -229,6 +229,51 @@ static struct nlmsghdr *nl_next(int s, char *buf, struct nlmsghdr *nh, ssize_t * > /* NOLINTNEXTLINE(readability-inconsistent-ifelse-braces) */\ > } else > > +/** > + * nl_collect() - Collect all respose Datagrams of a netlink multi part dump Nits: 'response', 'datagrams'. > + * @s: Netlink socket > + * @buf: Buffer for responses (at least NLBUFSIZ long) > + * @buflen: Buffer size. "Buffer size" > + * @seq: Sequence number of request we're getting responses for > + * > + * Return: Length of collected responses in bytes or error code < 0 Exceedingly minor, but for consistency (I strongly believe in boring code and comments): "length", ", negative error code" (see e.g. nl_do()). > + */ > +int nl_collect(int s, char *buf, size_t buflen, uint32_t seq) Cppcheck (try 'make cppcheck') says: netlink.c:242:5: style: The function 'nl_collect' should have static linkage since it is not used outside of its translation unit. [staticFunction] int nl_collect(int s, char *buf, size_t buflen, uint32_t seq) ^ > +{ > + size_t collected = 0; Coding style: we use tabs, see CONTRIBUTING.md. It's essentially the same coding style as the Linux kernel with the "netdev" variants. > + struct nlmsghdr *nh; Cppcheck reports that: netlink.c:245:22: style: The scope of the variable 'nh' can be reduced. [variableScope] struct nlmsghdr *nh; ^ and also: netlink.c:245:22: style: Variable 'nh' can be declared as pointer to const [constVariablePointer] struct nlmsghdr *nh; ^ > + ssize_t n; > + int res; > + const struct nlmsghdr* tail; ...and we also align variable declarations from the longest to the shortest (see references in CONTRIBUTING.md). Coding style: "struct nlmsghdr *tail". Same here for these two: netlink.c:247:9: style: The scope of the variable 'res' can be reduced. [variableScope] int res; ^ netlink.c:248:28: style: The scope of the variable 'tail' can be reduced. [variableScope] const struct nlmsghdr* tail; ^ > + > + #define NLMSG_DONE_SIZE NLMSG_LENGTH(4) It's just a header with no data, right? Shouldn't it be 0? Am I missing something? By the way we tend to group all the defines that are generally valid for a compilation unit towards the beginning of the relevant file (so that they can be used in all the functions), see #define directives earlier in netlink.c. Exceptions are macros that are only relevant or valid for a given function (see e.g. tap6_handler() in tap.c). > + > + while(collected + NLBUFSIZ < buflen){ Coding style: space before {. It's not really idiomatic to "do something as long as we have space" and, with this structure, we won't stop on errors or mismatching sequence numbers (see nl_status()). Would there be a way to encode this as a loop on nl_status() perhaps? I haven't tried but it would look cleaner and less bug-prone. > + nh = nl_next(s, &buf[collected], NULL, &n); > + debug("collect: add chunk of size n=%ld at offset=%ld, seq=%d nlmsg_len=%d", > + n, collected, seq, ((const struct nlmsghdr *)&buf[collected])->nlmsg_len); At some point, we should get a netlink monitor (see https://bugs.passt.top/show_bug.cgi?id=141) which might run this function quite frequently at runtime, and having this as debug() message would risk flooding debug logs and making them otherwise unusable. We have rate-limited versions of logging functions, but they wouldn't help debugging this function specifically, because once rate-limiting kicks in, the messages might become entirely useless. I think we could consider trace(). My preference would actually to drop this altogether because it seems to be specific to some particular phase of your development and not something of general relevance. If we want to keep this for some reason: "collect" isn't very descriptive (mind that it will just be printed among other messages coming from completely different parts of the code). I would rather use __func__. > + res = nl_status(nh, n, seq); > + if (res < 0) /* error */ > + return res; Looping on nl_status() directly would get rid of this. > + collected += n; > + > + /* look for NLMSG_DONE -- it's ussualy transmitted in a seperate datagram, usually, separate > + * but in some rare cases also placed at the end of a multipart chunk.*/ * [...] chunk. */ > + tail = (const struct nlmsghdr*)&buf[collected - NLMSG_DONE_SIZE]; Cppcheck and clang-tidy (try also 'make clang-tidy') aren't particularly impressed here. Cppcheck says: netlink.c:266:22: style: Variable 'tail' is reassigned a value before the old one has been used. [redundantAssignment] && (tail = (const struct nlmsghdr*)&buf[collected - NLMSG_DONE_SIZE]) ^ netlink.c:263:14: note: tail is assigned tail = (const struct nlmsghdr*)&buf[collected - NLMSG_DONE_SIZE]; ^ netlink.c:266:22: note: tail is overwritten && (tail = (const struct nlmsghdr*)&buf[collected - NLMSG_DONE_SIZE]) ^ and clang-tidy, more clearly, states: /home/sbrivio/passt/netlink.c:263:9: error: Value stored to 'tail' is never read [clang-analyzer-deadcode.DeadStores,-warnings-as-errors] 263 | tail = (const struct nlmsghdr*)&buf[collected - NLMSG_DONE_SIZE]; | ^ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ /home/sbrivio/passt/netlink.c:263:9: note: Value stored to 'tail' is never read 263 | tail = (const struct nlmsghdr*)&buf[collected - NLMSG_DONE_SIZE]; | ^ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > + if (res == 0 || ( > + collected >= NLMSG_DONE_SIZE > + && (tail = (const struct nlmsghdr*)&buf[collected - NLMSG_DONE_SIZE]) > + && NLMSG_OK(tail, NLMSG_DONE_SIZE) > + && (nl_status(tail, NLMSG_DONE_SIZE, seq) == 0)) For readability: could we perhaps split this into different 'if' conditions with gotos dealing with specific errors (for example if !NLMSG_OK(tail, NLMSG_DONE_SIZE), we should report error and return early I guess)? > + ) { > + debug("collected: %ld", collected); Same comments as the debug() call above. > + return collected; > + } > + } > + err("netlink: Too many routes to duplicate"); > + return -E2BIG; > +} > + > /** > * nl_do() - Send netlink "do" request, and wait for acknowledgement > * @s: Netlink socket > @@ -553,21 +598,21 @@ int nl_route_dup(int s_src, unsigned int ifi_src, > .rta.rta_len = RTA_LENGTH(sizeof(unsigned int)), > .ifi = ifi_src, > }; > - ssize_t nlmsgs_size, left, status; > + ssize_t nlmsgs_size, left, status = 0; > unsigned dup_routes = 0; > struct nlmsghdr *nh; > - char buf[NLBUFSIZ]; > + char buf[NLBUFSIZ * 8]; Reasonable, but perhaps it should be a #define just after #define NLBUFSIZ. If a change in a variable declaration changes its length (as displayed), you should move it so that we keep the "reverse pyramid" look. > uint32_t seq; > unsigned i; > > seq = nl_send(s_src, &req, RTM_GETROUTE, NLM_F_DUMP, sizeof(req)); > > - /* nl_foreach() will step through multiple response datagrams, > - * which we don't want here because we need to have all the > - * routes in the buffer at once. > - */ > - nh = nl_next(s_src, buf, NULL, &nlmsgs_size); > - for (left = nlmsgs_size; > + /* collect all the chunks of a multi part dump in one buffer.*/ /* Collect [...] buffer */ > + nlmsgs_size = nl_collect(s_src, buf, sizeof(buf), seq); > + if (nlmsgs_size < 0) > + return nlmsgs_size; > + > + for (nh = (struct nlmsghdr *)buf, left = nlmsgs_size; > NLMSG_OK(nh, left) && (status = nl_status(nh, left, seq)) > 0; > nh = NLMSG_NEXT(nh, left)) { > struct rtmsg *rtm = (struct rtmsg *)NLMSG_DATA(nh); > @@ -648,24 +693,12 @@ int nl_route_dup(int s_src, unsigned int ifi_src, > dup_routes++; > } > > - if (!NLMSG_OK(nh, left)) { > - /* Process any remaining datagrams in a different > - * buffer so we don't overwrite the first one. > - */ > - char tail[NLBUFSIZ]; > - unsigned extra = 0; > - > - nl_foreach_oftype(nh, status, s_src, tail, seq, RTM_NEWROUTE) > - extra++; > - > - if (extra) { > - err("netlink: Too many routes to duplicate"); > - return -E2BIG; > - } > - } > + /* status should be 0 (=NLMSG_DONE) */ This comment is redundant: if status is not 0, then clearly we hit some kind of issue. > if (status < 0) > return status; > > + debug("found %d routes in dump", dup_routes); Same comments as debug() calls above. > + > /* Routes might have dependencies between each other, and the kernel > * processes RTM_NEWROUTE messages sequentially. For n routes, we might > * need to send the requests up to n times to get all of them inserted. -- Stefano