Re: [PATCH 1/3] musl adaptations

[Stefano Brivio <sbrivio@redhat.com>]

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