[PATCH 1/3] musl adaptations

[PATCH 1/3] musl adaptations

[KuhnChris]

---
 conf.c  |  6 +++---
 passt.c |  2 +-
 passt.h |  4 ++--
 util.h  | 14 ++++++++++++++
 4 files changed, 20 insertions(+), 6 deletions(-)

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 <stdbool.h>
-
+#include <bits/limits.h>
 #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;
 	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 <stdlib.h>
+#include <stdio.h>
 #include <stdarg.h>
+#include <signal.h>
+#include <byteswap.h>
 
 #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__)
+
 #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) \
-- 
@@ -7,7 +7,10 @@
 #define UTIL_H
 
 #include <stdlib.h>
+#include <stdio.h>
 #include <stdarg.h>
+#include <signal.h>
+#include <byteswap.h>
 
 #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__)
+
 #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) \
-- 
2.38.4

[PATCH 2/3] sbrivio/manual buffer size due to musl being 1024

[KuhnChris]

---
 netlink.c | 15 +++++++++------
 1 file changed, 9 insertions(+), 6 deletions(-)

diff --git a/netlink.c b/netlink.c
index 8f785ca..0e0be4f 100644
--- a/netlink.c
+++ b/netlink.c
@@ -34,6 +34,8 @@
 #include "log.h"
 #include "netlink.h"
 
+#define NLBUFSIZ	(8192 * sizeof(struct nlmsghdr)) /* See netlink(7) */
+
 /* Socket in init, in target namespace, sequence (just needs to be monotonic) */
 static int nl_sock	= -1;
 static int nl_sock_ns	= -1;
@@ -105,7 +107,7 @@ fail:
 static int nl_req(int ns, char *buf, const void *req, ssize_t len)
 {
 	int s = ns ? nl_sock_ns : nl_sock, done = 0;
-	char flush[BUFSIZ];
+	char flush[NLBUFSIZ];
 	ssize_t n;
 
 	while (!done && (n = recv(s, flush, sizeof(flush), MSG_DONTWAIT)) > 0) {
@@ -121,7 +123,8 @@ static int nl_req(int ns, char *buf, const void *req, ssize_t len)
 		}
 	}
 
-	if ((send(s, req, len, 0) < len) || (len = recv(s, buf, BUFSIZ, 0)) < 0)
+	if ((send(s, req, len, 0) < len) ||
+	    (len = recv(s, buf, NLBUFSIZ, 0)) < 0)
 		return -errno;
 
 	return len;
@@ -149,7 +152,7 @@ unsigned int nl_get_ext_if(sa_family_t af)
 	};
 	struct nlmsghdr *nh;
 	struct rtattr *rta;
-	char buf[BUFSIZ];
+	char buf[NLBUFSIZ];
 	ssize_t n;
 	size_t na;
 
@@ -227,7 +230,7 @@ void nl_route(int ns, unsigned int ifi, sa_family_t af, void *gw)
 	struct nlmsghdr *nh;
 	struct rtattr *rta;
 	struct rtmsg *rtm;
-	char buf[BUFSIZ];
+	char buf[NLBUFSIZ];
 	ssize_t n;
 	size_t na;
 
@@ -336,7 +339,7 @@ void nl_addr(int ns, unsigned int ifi, sa_family_t af,
 	struct ifaddrmsg *ifa;
 	struct nlmsghdr *nh;
 	struct rtattr *rta;
-	char buf[BUFSIZ];
+	char buf[NLBUFSIZ];
 	ssize_t n;
 	size_t na;
 
@@ -446,7 +449,7 @@ void nl_link(int ns, unsigned int ifi, void *mac, int up, int mtu)
 	struct ifinfomsg *ifm;
 	struct nlmsghdr *nh;
 	struct rtattr *rta;
-	char buf[BUFSIZ];
+	char buf[NLBUFSIZ];
 	ssize_t n;
 	size_t na;
 
-- 
@@ -34,6 +34,8 @@
 #include "log.h"
 #include "netlink.h"
 
+#define NLBUFSIZ	(8192 * sizeof(struct nlmsghdr)) /* See netlink(7) */
+
 /* Socket in init, in target namespace, sequence (just needs to be monotonic) */
 static int nl_sock	= -1;
 static int nl_sock_ns	= -1;
@@ -105,7 +107,7 @@ fail:
 static int nl_req(int ns, char *buf, const void *req, ssize_t len)
 {
 	int s = ns ? nl_sock_ns : nl_sock, done = 0;
-	char flush[BUFSIZ];
+	char flush[NLBUFSIZ];
 	ssize_t n;
 
 	while (!done && (n = recv(s, flush, sizeof(flush), MSG_DONTWAIT)) > 0) {
@@ -121,7 +123,8 @@ static int nl_req(int ns, char *buf, const void *req, ssize_t len)
 		}
 	}
 
-	if ((send(s, req, len, 0) < len) || (len = recv(s, buf, BUFSIZ, 0)) < 0)
+	if ((send(s, req, len, 0) < len) ||
+	    (len = recv(s, buf, NLBUFSIZ, 0)) < 0)
 		return -errno;
 
 	return len;
@@ -149,7 +152,7 @@ unsigned int nl_get_ext_if(sa_family_t af)
 	};
 	struct nlmsghdr *nh;
 	struct rtattr *rta;
-	char buf[BUFSIZ];
+	char buf[NLBUFSIZ];
 	ssize_t n;
 	size_t na;
 
@@ -227,7 +230,7 @@ void nl_route(int ns, unsigned int ifi, sa_family_t af, void *gw)
 	struct nlmsghdr *nh;
 	struct rtattr *rta;
 	struct rtmsg *rtm;
-	char buf[BUFSIZ];
+	char buf[NLBUFSIZ];
 	ssize_t n;
 	size_t na;
 
@@ -336,7 +339,7 @@ void nl_addr(int ns, unsigned int ifi, sa_family_t af,
 	struct ifaddrmsg *ifa;
 	struct nlmsghdr *nh;
 	struct rtattr *rta;
-	char buf[BUFSIZ];
+	char buf[NLBUFSIZ];
 	ssize_t n;
 	size_t na;
 
@@ -446,7 +449,7 @@ void nl_link(int ns, unsigned int ifi, void *mac, int up, int mtu)
 	struct ifinfomsg *ifm;
 	struct nlmsghdr *nh;
 	struct rtattr *rta;
-	char buf[BUFSIZ];
+	char buf[NLBUFSIZ];
 	ssize_t n;
 	size_t na;
 
-- 
2.38.4

Re: [PATCH 2/3] sbrivio/manual buffer size due to musl being 1024

[Stefano Brivio]

On Fri,  3 Mar 2023 22:49:30 +0000
KuhnChris <kuhnchris+github@kuhnchris.eu> wrote:

> ---
>  netlink.c | 15 +++++++++------
>  1 file changed, 9 insertions(+), 6 deletions(-)

This patch should also come with a commit message. You can write what I
explained on the ticket, and explain (in plain English) that this patch
comes from me. Or even just add:

From: Stefano Brivio <sbrivio@redhat.com>

at the beginning of the commit message.

The rest here looks good to me (of course, I wrote it :D).

-- 
Stefano

[PATCH 3/3] define _bswap_constant_16/32 for musl This uses the implementation from libiio (https://github.com/analogdevicesinc/libiio/commit/9c6c6a432a0cbe2832bc97a7eeddfb61f6b8b856) Also restored the old behavior from the old util.h.

[KuhnChris]

---
 util.h | 19 ++++++++++---------
 1 file changed, 10 insertions(+), 9 deletions(-)

diff --git a/util.h b/util.h
index 7315ce2..a6ad4ad 100644
--- a/util.h
+++ b/util.h
@@ -91,7 +91,16 @@
 #define MAC_ZERO		((uint8_t [ETH_ALEN]){ 0 })
 #define MAC_IS_ZERO(addr)	(!memcmp((addr), MAC_ZERO, ETH_ALEN))
 
-#if defined(__GLIBC__) || defined(__UCLIBC__)
+#ifndef __bswap_constant_16
+#define __bswap_constant_16(x) \
+	((unsigned short int) ((((x) >> 8) & 0xff) | (((x) & 0xff) << 8)))
+#endif
+
+#ifndef __bswap_constant_32
+#define __bswap_constant_32(x) \
+	((((x) & 0xff000000) >> 24) | (((x) & 0x00ff0000) >>  8) | \
+	 (((x) & 0x0000ff00) <<  8) | (((x) & 0x000000ff) << 24))
+#endif
 
 #if __BYTE_ORDER == __BIG_ENDIAN
 #define	htons_constant(x)	(x)
@@ -101,14 +110,6 @@
 #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))
-- 
@@ -91,7 +91,16 @@
 #define MAC_ZERO		((uint8_t [ETH_ALEN]){ 0 })
 #define MAC_IS_ZERO(addr)	(!memcmp((addr), MAC_ZERO, ETH_ALEN))
 
-#if defined(__GLIBC__) || defined(__UCLIBC__)
+#ifndef __bswap_constant_16
+#define __bswap_constant_16(x) \
+	((unsigned short int) ((((x) >> 8) & 0xff) | (((x) & 0xff) << 8)))
+#endif
+
+#ifndef __bswap_constant_32
+#define __bswap_constant_32(x) \
+	((((x) & 0xff000000) >> 24) | (((x) & 0x00ff0000) >>  8) | \
+	 (((x) & 0x0000ff00) <<  8) | (((x) & 0x000000ff) << 24))
+#endif
 
 #if __BYTE_ORDER == __BIG_ENDIAN
 #define	htons_constant(x)	(x)
@@ -101,14 +110,6 @@
 #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))
-- 
2.38.4

Re: [PATCH 3/3] define _bswap_constant_16/32 for musl This uses the implementation from libiio (https://github.com/analogdevicesinc/libiio/commit/9c6c6a432a0cbe2832bc97a7eeddfb61f6b8b856) Also restored the old behavior from the old util.h.

[Stefano Brivio]

On Fri,  3 Mar 2023 22:49:31 +0000
KuhnChris <kuhnchris+github@kuhnchris.eu> wrote:

> ---
>  util.h | 19 ++++++++++---------
>  1 file changed, 10 insertions(+), 9 deletions(-)

Here, what you wrote in the subject (first line of commit message)
should be in the commit message itself. Example of commit message for
this:

--
util: Provide own __bswap_constant_{16,32} if not defined by C library

musl, and possibly other C libraries, don't define __bswap_constant_16
and __bswap_constant_32. Define them if they aren't.

Signed-off-by: Name Surname <email address>
--

The first line becomes the patch subject. Here, there would be no
strict need to mention libiio because that's just one example of the
same trivial implementation copied over and over again in many
projects, which might come from pretty much anywhere.

Wwas it copied from glibc? Was it written by the author of that
commit? I don't think it qualifies as copyrightable work:

  https://en.wikipedia.org/wiki/Threshold_of_originality

...but you can also do that, as you have the link at hand, if you
actually took it from there.

> diff --git a/util.h b/util.h
> index 7315ce2..a6ad4ad 100644
> --- a/util.h
> +++ b/util.h
> @@ -91,7 +91,16 @@
>  #define MAC_ZERO		((uint8_t [ETH_ALEN]){ 0 })
>  #define MAC_IS_ZERO(addr)	(!memcmp((addr), MAC_ZERO, ETH_ALEN))
>  
> -#if defined(__GLIBC__) || defined(__UCLIBC__)
> +#ifndef __bswap_constant_16
> +#define __bswap_constant_16(x) \
> +	((unsigned short int) ((((x) >> 8) & 0xff) | (((x) & 0xff) << 8)))
> +#endif
> +
> +#ifndef __bswap_constant_32
> +#define __bswap_constant_32(x) \
> +	((((x) & 0xff000000) >> 24) | (((x) & 0x00ff0000) >>  8) | \
> +	 (((x) & 0x0000ff00) <<  8) | (((x) & 0x000000ff) << 24))
> +#endif
>  
>  #if __BYTE_ORDER == __BIG_ENDIAN
>  #define	htons_constant(x)	(x)
> @@ -101,14 +110,6 @@
>  #define	htonl_constant(x)	(__bswap_constant_32(x))
>  #endif
>  
> -#else 
> -
> -/* mainly musl fallback */
> -
> -#define	htons_constant(x)	(x)
> -#define	htonl_constant(x)	(x)
> -
> -#endif

And this, again, shouldn't be in the first patch.

>  
>  #define IN4_IS_ADDR_UNSPECIFIED(a) \
>  	((a)->s_addr == htonl(INADDR_ANY))

I understand this might be a lot to digest and a bit of effort, so let
me know -- you figured out most of the issues and did all the tests, I
can also fix up the patches for you.

-- 
Stefano

Re: [PATCH 1/3] musl adaptations

[Stefano Brivio]

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 <kuhnchris+github@kuhnchris.eu> 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 <stdbool.h>
> -
> +#include <bits/limits.h>

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 <stdlib.h>
> +#include <stdio.h>
>  #include <stdarg.h>
> +#include <signal.h>
> +#include <byteswap.h>

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