From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.129.124]) by passt.top (Postfix) with ESMTP id E25825A0262 for ; Sat, 4 Mar 2023 10:54:36 +0100 (CET) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1677923675; 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=8fGL4jOcwX8qDmF54AzMJBG4dHYVWAALAVGq0Rw419k=; b=HVzCtB78ZEZqQu43tMAUDA4/NLCJaF2mRIkpYAP9FU4IBMf2r9SvyugLl1ejICAfL0fPPu aEzmuWoILqYicinDUobWfZugaxMNV+3DBRMTTI37LHMKA0zIDsY/o/i/H4PU2Kn28goHtP yDYIxP5YYJ/D7GLWnItmpLvFBVZ07+o= Received: from mimecast-mx02.redhat.com (mimecast-mx02.redhat.com [66.187.233.88]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id us-mta-117-tAuStVV0N4u_E8ajgCE7eQ-1; Sat, 04 Mar 2023 04:54:34 -0500 X-MC-Unique: tAuStVV0N4u_E8ajgCE7eQ-1 Received: from smtp.corp.redhat.com (int-mx06.intmail.prod.int.rdu2.redhat.com [10.11.54.6]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx02.redhat.com (Postfix) with ESMTPS id C9349802D2F; Sat, 4 Mar 2023 09:54:33 +0000 (UTC) Received: from maya.cloud.tilaa.com (unknown [10.33.32.23]) by smtp.corp.redhat.com (Postfix) with ESMTPS id 3E6DA2166B26; Sat, 4 Mar 2023 09:54:33 +0000 (UTC) Date: Sat, 4 Mar 2023 10:54:30 +0100 From: Stefano Brivio To: KuhnChris Subject: Re: [PATCH 1/3] musl adaptations Message-ID: <20230304105430.30872a1c@elisabeth> In-Reply-To: <20230303224931.11791-1-kuhnchris+github@kuhnchris.eu> References: <20230303224931.11791-1-kuhnchris+github@kuhnchris.eu> Organization: Red Hat MIME-Version: 1.0 X-Scanned-By: MIMEDefang 3.1 on 10.11.54.6 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Message-ID-Hash: SX3AWZU23AUU6LPJYZAPFVMW7B5HM3DO X-Message-ID-Hash: SX3AWZU23AUU6LPJYZAPFVMW7B5HM3DO 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: Hi Chris, Thanks for the effort and the patches! Context for others: this comes from https://bugs.passt.top/show_bug.cgi?id=4. I can also take it from here. But if you want to send proper patches, these items need to be fixed, see below: On Fri, 3 Mar 2023 22:49:29 +0000 KuhnChris wrote: > --- > conf.c | 6 +++--- > passt.c | 2 +- > passt.h | 4 ++-- > util.h | 14 ++++++++++++++ > 4 files changed, 20 insertions(+), 6 deletions(-) This needs a commit message. See the git log or list archives: https://archives.passt.top/passt-dev/ for how patches are typically submitted. Here, you should mention the issue with "stderr" and musl. And the rest of the changes should be in separate patches, I think. Rationale: if somebody figures out how to fix the issue with "stderr", one day, we can just revert a single patch. It's also easier for others to just focus on that while reviewing. There are a lot of advantages in terms of maintainability, really. Also, for series, you would usually send a cover letter (git format-patch --cover-letter) with a short description of what the series does. Or even nothing, if the subject is clear enough. But for example, one important information you should mention there is the architectures and environments where you tested this. You also need to add your "Signed-off-by:" (in the commit message), see for example: https://www.kernel.org/doc/html/latest/process/submitting-patches.html?highlight=signed%20off#developer-s-certificate-of-origin-1-1 now, this is not the Linux kernel, but we follow pretty much the same rules. > diff --git a/conf.c b/conf.c > index 0e512f4..c1dd9ba 100644 > --- a/conf.c > +++ b/conf.c > @@ -1305,13 +1305,13 @@ void conf(struct ctx *c, int argc, char **argv) > if (logfile) > die("Can't log to both file and stderr"); > > - if (c->stderr) > + if (c->force_stderr) > die("Multiple --stderr options given"); > > - c->stderr = 1; > + c->force_stderr = 1; > break; > case 'l': > - if (c->stderr) > + if (c->force_stderr) > die("Can't log to both stderr and file"); > > if (logfile) > diff --git a/passt.c b/passt.c > index 5b8146e..f67213a 100644 > --- a/passt.c > +++ b/passt.c > @@ -241,7 +241,7 @@ int main(int argc, char **argv) > conf(&c, argc, argv); > trace_init(c.trace); > > - if (c.stderr || isatty(fileno(stdout))) > + if (c.force_stderr || isatty(fileno(stdout))) > __openlog(log_name, LOG_PERROR, LOG_DAEMON); > > quit_fd = pasta_netns_quit_init(&c); > diff --git a/passt.h b/passt.h > index e0383eb..71d3602 100644 > --- a/passt.h > +++ b/passt.h > @@ -32,7 +32,7 @@ struct tap_l4_msg { > union epoll_ref; > > #include > - > +#include This should be in a separate patch where you fix headers inclusion. And you shouldn't remove the blank line before it: it separates system headers from local headers. > #include "packet.h" > #include "icmp.h" > #include "port_fwd.h" > @@ -197,7 +197,7 @@ struct ctx { > int trace; > int quiet; > int foreground; > - int stderr; > + int force_stderr; ...and with this, you should also change the comment before struct ctx. > int nofile; > char sock_path[UNIX_PATH_MAX]; > char pcap[PATH_MAX]; > diff --git a/util.h b/util.h > index 570094c..7315ce2 100644 > --- a/util.h > +++ b/util.h > @@ -7,7 +7,10 @@ > #define UTIL_H > > #include > +#include > #include > +#include > +#include Also in a separate patch. > > #include "log.h" > > @@ -88,6 +91,8 @@ > #define MAC_ZERO ((uint8_t [ETH_ALEN]){ 0 }) > #define MAC_IS_ZERO(addr) (!memcmp((addr), MAC_ZERO, ETH_ALEN)) > > +#if defined(__GLIBC__) || defined(__UCLIBC__) > + It goes away in 3/3, no need to add it here. > #if __BYTE_ORDER == __BIG_ENDIAN > #define htons_constant(x) (x) > #define htonl_constant(x) (x) > @@ -96,6 +101,15 @@ > #define htonl_constant(x) (__bswap_constant_32(x)) > #endif > > +#else > + > +/* mainly musl fallback */ > + > +#define htons_constant(x) (x) > +#define htonl_constant(x) (x) > + > +#endif > + > #define IN4_IS_ADDR_UNSPECIFIED(a) \ > ((a)->s_addr == htonl(INADDR_ANY)) > #define IN4_IS_ADDR_BROADCAST(a) \ -- Stefano