---
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) \
--
2.38.4
---
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;
--
2.38.4
On Fri, 3 Mar 2023 22:49:30 +0000 KuhnChris <kuhnchris+github(a)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(a)redhat.com> at the beginning of the commit message. The rest here looks good to me (of course, I wrote it :D). -- Stefano
---
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))
--
2.38.4
On Fri, 3 Mar 2023 22:49:31 +0000 KuhnChris <kuhnchris+github(a)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) - -#endifAnd 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
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(a)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?high… 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