* [PATCH v2 1/9] log to stderr until process is daemonized, even if a logfile is set
2023-02-08 17:48 [PATCH v2 0/9] error logging fixes Laine Stump
@ 2023-02-08 17:48 ` Laine Stump
2023-02-09 17:45 ` Stefano Brivio
2023-02-08 17:48 ` [PATCH v2 2/9] add errexit() to log an error message and exit with a single call Laine Stump
` (7 subsequent siblings)
8 siblings, 1 reply; 21+ messages in thread
From: Laine Stump @ 2023-02-08 17:48 UTC (permalink / raw)
To: passt-dev, laine
Signed-off-by: Laine Stump <laine@redhat.com>
---
log.c | 14 +++++++++++++-
log.h | 1 +
passt.c | 6 ++++--
3 files changed, 18 insertions(+), 3 deletions(-)
diff --git a/log.c b/log.c
index 468c730..0ab0adf 100644
--- a/log.c
+++ b/log.c
@@ -34,6 +34,7 @@ static int log_sock = -1; /* Optional socket to system logger */
static char log_ident[BUFSIZ]; /* Identifier string for openlog() */
static int log_mask; /* Current log priority mask */
static int log_opt; /* Options for openlog() */
+static int log_daemon_mode = false; /* true once process is daemonized */
static int log_file = -1; /* Optional log file descriptor */
static size_t log_size; /* Maximum log file size in bytes */
@@ -67,7 +68,8 @@ void name(const char *format, ...) { \
} \
\
if ((setlogmask(0) & LOG_MASK(LOG_DEBUG) || \
- setlogmask(0) == LOG_MASK(LOG_EMERG)) && log_file == -1) { \
+ setlogmask(0) == LOG_MASK(LOG_EMERG)) \
+ && (log_file == -1 || !log_daemon_mode)) { \
va_start(args, format); \
(void)vfprintf(stderr, format, args); \
va_end(args); \
@@ -91,6 +93,16 @@ logfn(warn, LOG_WARNING)
logfn(info, LOG_INFO)
logfn(debug, LOG_DEBUG)
+/**
+ * log_go_daemon() - tell logging subsystem that the process has been
+ * been daemonized, so it stop logging to stderr
+ * if appropriate.
+ */
+void log_go_daemon(void)
+{
+ log_daemon_mode = true;
+}
+
/**
* trace_init() - Set log_trace depending on trace (debug) mode
* @enable: Tracing debug mode enabled if non-zero
diff --git a/log.h b/log.h
index 987dc17..a57c777 100644
--- a/log.h
+++ b/log.h
@@ -25,6 +25,7 @@ void trace_init(int enable);
void __openlog(const char *ident, int option, int facility);
void logfile_init(const char *name, const char *path, size_t size);
+void log_go_daemon(void);
void passt_vsyslog(int pri, const char *format, va_list ap);
void logfile_write(int pri, const char *format, va_list ap);
void __setlogmask(int mask);
diff --git a/passt.c b/passt.c
index 8b2c50d..cf010e8 100644
--- a/passt.c
+++ b/passt.c
@@ -296,10 +296,12 @@ int main(int argc, char **argv)
exit(EXIT_FAILURE);
}
- if (!c.foreground)
+ if (!c.foreground) {
__daemon(pidfile_fd, devnull_fd);
- else
+ log_go_daemon();
+ } else {
write_pidfile(pidfile_fd, getpid());
+ }
isolate_postfork(&c);
--
@@ -296,10 +296,12 @@ int main(int argc, char **argv)
exit(EXIT_FAILURE);
}
- if (!c.foreground)
+ if (!c.foreground) {
__daemon(pidfile_fd, devnull_fd);
- else
+ log_go_daemon();
+ } else {
write_pidfile(pidfile_fd, getpid());
+ }
isolate_postfork(&c);
--
2.39.1
^ permalink raw reply related [flat|nested] 21+ messages in thread* Re: [PATCH v2 1/9] log to stderr until process is daemonized, even if a logfile is set
2023-02-08 17:48 ` [PATCH v2 1/9] log to stderr until process is daemonized, even if a logfile is set Laine Stump
@ 2023-02-09 17:45 ` Stefano Brivio
2023-02-14 3:41 ` Laine Stump
0 siblings, 1 reply; 21+ messages in thread
From: Stefano Brivio @ 2023-02-09 17:45 UTC (permalink / raw)
To: Laine Stump; +Cc: passt-dev, laine
On Wed, 8 Feb 2023 12:48:30 -0500
Laine Stump <laine@redhat.com> wrote:
> Signed-off-by: Laine Stump <laine@redhat.com>
> ---
> log.c | 14 +++++++++++++-
> log.h | 1 +
> passt.c | 6 ++++--
> 3 files changed, 18 insertions(+), 3 deletions(-)
>
> diff --git a/log.c b/log.c
> index 468c730..0ab0adf 100644
> --- a/log.c
> +++ b/log.c
> @@ -34,6 +34,7 @@ static int log_sock = -1; /* Optional socket to system logger */
> static char log_ident[BUFSIZ]; /* Identifier string for openlog() */
> static int log_mask; /* Current log priority mask */
> static int log_opt; /* Options for openlog() */
> +static int log_daemon_mode = false; /* true once process is daemonized */
>
> static int log_file = -1; /* Optional log file descriptor */
> static size_t log_size; /* Maximum log file size in bytes */
> @@ -67,7 +68,8 @@ void name(const char *format, ...) { \
> } \
> \
> if ((setlogmask(0) & LOG_MASK(LOG_DEBUG) || \
> - setlogmask(0) == LOG_MASK(LOG_EMERG)) && log_file == -1) { \
> + setlogmask(0) == LOG_MASK(LOG_EMERG)) \
> + && (log_file == -1 || !log_daemon_mode)) { \
This is getting a bit complicated.
At the moment, LOG_EMERG is abused with the meaning "we don't know
where/what to log yet", because we didn't process logging options yet.
Would it be an option to extend this abuse a bit further, and use
LOG_EMERG to indicate that passt didn't daemonise yet, instead? You
would just need to move the __setlogmask() calls in main(), and change
the comment about the first one.
I haven't tested this, and we should be a bit careful with this (check
what happens with and without running passt from an interactive
terminal, with and without a log file, etc.).
A possibly cleaner approach could be to decouple this from the log
mask, and have an enum instead, to represent logging "states" or
"modes" -- but I'm not sure we're really going to save much complexity
with it.
By the way, the man page should be updated as well.
--
Stefano
^ permalink raw reply [flat|nested] 21+ messages in thread* Re: [PATCH v2 1/9] log to stderr until process is daemonized, even if a logfile is set
2023-02-09 17:45 ` Stefano Brivio
@ 2023-02-14 3:41 ` Laine Stump
0 siblings, 0 replies; 21+ messages in thread
From: Laine Stump @ 2023-02-14 3:41 UTC (permalink / raw)
To: passt-dev; +Cc: Stefano Brivio
On 2/9/23 12:45 PM, Stefano Brivio wrote:
> On Wed, 8 Feb 2023 12:48:30 -0500
> Laine Stump <laine@redhat.com> wrote:
>
>> Signed-off-by: Laine Stump <laine@redhat.com>
>> ---
>> log.c | 14 +++++++++++++-
>> log.h | 1 +
>> passt.c | 6 ++++--
>> 3 files changed, 18 insertions(+), 3 deletions(-)
>>
>> diff --git a/log.c b/log.c
>> index 468c730..0ab0adf 100644
>> --- a/log.c
>> +++ b/log.c
>> @@ -34,6 +34,7 @@ static int log_sock = -1; /* Optional socket to system logger */
>> static char log_ident[BUFSIZ]; /* Identifier string for openlog() */
>> static int log_mask; /* Current log priority mask */
>> static int log_opt; /* Options for openlog() */
>> +static int log_daemon_mode = false; /* true once process is daemonized */
>>
>> static int log_file = -1; /* Optional log file descriptor */
>> static size_t log_size; /* Maximum log file size in bytes */
>> @@ -67,7 +68,8 @@ void name(const char *format, ...) { \
>> } \
>> \
>> if ((setlogmask(0) & LOG_MASK(LOG_DEBUG) || \
>> - setlogmask(0) == LOG_MASK(LOG_EMERG)) && log_file == -1) { \
>> + setlogmask(0) == LOG_MASK(LOG_EMERG)) \
>> + && (log_file == -1 || !log_daemon_mode)) { \
>
> This is getting a bit complicated.
>
> At the moment, LOG_EMERG is abused with the meaning "we don't know
> where/what to log yet", because we didn't process logging options yet.
>
> Would it be an option to extend this abuse a bit further, and use
> LOG_EMERG to indicate that passt didn't daemonise yet, instead? You
> would just need to move the __setlogmask() calls in main(), and change
> the comment about the first one.
Well, the fact that I didn't understand this was what was being done
with the log mask kind of indicates this abuse will be misunderstood in
the long run :-P
But the abuse is already there, so it's not like I'm really making it
any (much?) worse. It's just increasing the window when the logmask is
set to LOG_EMERG, while modifying the log macro to not care if log_file
is opened or not. I'll try that and see how it works out.
>
> I haven't tested this, and we should be a bit careful with this (check
> what happens with and without running passt from an interactive
> terminal, with and without a log file, etc.).
>
> A possibly cleaner approach could be to decouple this from the log
> mask, and have an enum instead, to represent logging "states" or
> "modes" -- but I'm not sure we're really going to save much complexity
> with it.
If I'm understanding you, that's kind of what I was doing, it's just
that there are two modes - false == "logging before being daemonized"
and true == "logging after being daemonized".
>
> By the way, the man page should be updated as well.
>
Good point.
^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH v2 2/9] add errexit() to log an error message and exit with a single call
2023-02-08 17:48 [PATCH v2 0/9] error logging fixes Laine Stump
2023-02-08 17:48 ` [PATCH v2 1/9] log to stderr until process is daemonized, even if a logfile is set Laine Stump
@ 2023-02-08 17:48 ` Laine Stump
2023-02-09 17:45 ` Stefano Brivio
2023-02-08 17:48 ` [PATCH v2 3/9] eliminate most calls to usage() in conf() Laine Stump
` (6 subsequent siblings)
8 siblings, 1 reply; 21+ messages in thread
From: Laine Stump @ 2023-02-08 17:48 UTC (permalink / raw)
To: passt-dev, laine
Almost all occurences of err() are either immediately followed by
exit(EXIT_FAILURE), usage(argv[0]) (which itself then calls
exit(EXIT_FAILURE), or that is what's done immediately after returning
from the function that calls err(). Modify the errfn macro so that its
instantiations can include exit(EXIT_FAILURE) at the end, and use that
to create a new function errxit() that will log an error and then
exit.
Signed-off-by: Laine Stump <laine@redhat.com>
---
log.c | 13 ++++++++-----
log.h | 1 +
2 files changed, 9 insertions(+), 5 deletions(-)
diff --git a/log.c b/log.c
index 0ab0adf..4956914 100644
--- a/log.c
+++ b/log.c
@@ -45,7 +45,7 @@ static char log_header[BUFSIZ]; /* File header, written back on cuts */
static time_t log_start; /* Start timestamp */
int log_trace; /* --trace mode enabled */
-#define logfn(name, level) \
+#define logfn(name, level, doexit) \
void name(const char *format, ...) { \
struct timespec tp; \
va_list args; \
@@ -76,6 +76,8 @@ void name(const char *format, ...) { \
if (format[strlen(format)] != '\n') \
fprintf(stderr, "\n"); \
} \
+ if (doexit) \
+ exit(EXIT_FAILURE); \
}
/* Prefixes for log file messages, indexed by priority */
@@ -88,10 +90,11 @@ const char *logfile_prefix[] = {
" ", /* LOG_DEBUG */
};
-logfn(err, LOG_ERR)
-logfn(warn, LOG_WARNING)
-logfn(info, LOG_INFO)
-logfn(debug, LOG_DEBUG)
+logfn(errexit, LOG_ERR, 1)
+logfn(err, LOG_ERR, 0)
+logfn(warn, LOG_WARNING, 0)
+logfn(info, LOG_INFO, 0)
+logfn(debug, LOG_DEBUG, 0)
/**
* log_go_daemon() - tell logging subsystem that the process has been
diff --git a/log.h b/log.h
index a57c777..ed19415 100644
--- a/log.h
+++ b/log.h
@@ -10,6 +10,7 @@
#define LOGFILE_CUT_RATIO 30 /* When full, cut ~30% size */
#define LOGFILE_SIZE_MIN (5UL * MAX(BUFSIZ, PAGE_SIZE))
+void errexit(const char *format, ...);
void err(const char *format, ...);
void warn(const char *format, ...);
void info(const char *format, ...);
--
@@ -10,6 +10,7 @@
#define LOGFILE_CUT_RATIO 30 /* When full, cut ~30% size */
#define LOGFILE_SIZE_MIN (5UL * MAX(BUFSIZ, PAGE_SIZE))
+void errexit(const char *format, ...);
void err(const char *format, ...);
void warn(const char *format, ...);
void info(const char *format, ...);
--
2.39.1
^ permalink raw reply related [flat|nested] 21+ messages in thread* Re: [PATCH v2 2/9] add errexit() to log an error message and exit with a single call
2023-02-08 17:48 ` [PATCH v2 2/9] add errexit() to log an error message and exit with a single call Laine Stump
@ 2023-02-09 17:45 ` Stefano Brivio
2023-02-13 3:22 ` David Gibson
0 siblings, 1 reply; 21+ messages in thread
From: Stefano Brivio @ 2023-02-09 17:45 UTC (permalink / raw)
To: Laine Stump; +Cc: passt-dev, laine
On Wed, 8 Feb 2023 12:48:31 -0500
Laine Stump <laine@redhat.com> wrote:
> Almost all occurences of err() are either immediately followed by
> exit(EXIT_FAILURE), usage(argv[0]) (which itself then calls
> exit(EXIT_FAILURE), or that is what's done immediately after returning
> from the function that calls err(). Modify the errfn macro so that its
> instantiations can include exit(EXIT_FAILURE) at the end, and use that
> to create a new function errxit() that will log an error and then
> exit.
>
> Signed-off-by: Laine Stump <laine@redhat.com>
> ---
> log.c | 13 ++++++++-----
> log.h | 1 +
> 2 files changed, 9 insertions(+), 5 deletions(-)
>
> diff --git a/log.c b/log.c
> index 0ab0adf..4956914 100644
> --- a/log.c
> +++ b/log.c
> @@ -45,7 +45,7 @@ static char log_header[BUFSIZ]; /* File header, written back on cuts */
> static time_t log_start; /* Start timestamp */
> int log_trace; /* --trace mode enabled */
>
> -#define logfn(name, level) \
> +#define logfn(name, level, doexit) \
> void name(const char *format, ...) { \
> struct timespec tp; \
> va_list args; \
> @@ -76,6 +76,8 @@ void name(const char *format, ...) { \
> if (format[strlen(format)] != '\n') \
> fprintf(stderr, "\n"); \
> } \
> + if (doexit) \
A blank line before this would make it more consistent.
> + exit(EXIT_FAILURE); \
> }
>
> /* Prefixes for log file messages, indexed by priority */
> @@ -88,10 +90,11 @@ const char *logfile_prefix[] = {
> " ", /* LOG_DEBUG */
> };
>
> -logfn(err, LOG_ERR)
> -logfn(warn, LOG_WARNING)
> -logfn(info, LOG_INFO)
> -logfn(debug, LOG_DEBUG)
> +logfn(errexit, LOG_ERR, 1)
> +logfn(err, LOG_ERR, 0)
> +logfn(warn, LOG_WARNING, 0)
> +logfn(info, LOG_INFO, 0)
> +logfn(debug, LOG_DEBUG, 0)
>
> /**
> * log_go_daemon() - tell logging subsystem that the process has been
> diff --git a/log.h b/log.h
> index a57c777..ed19415 100644
> --- a/log.h
> +++ b/log.h
> @@ -10,6 +10,7 @@
> #define LOGFILE_CUT_RATIO 30 /* When full, cut ~30% size */
> #define LOGFILE_SIZE_MIN (5UL * MAX(BUFSIZ, PAGE_SIZE))
>
> +void errexit(const char *format, ...);
> void err(const char *format, ...);
> void warn(const char *format, ...);
> void info(const char *format, ...);
Other than that, this looks good to me.
--
Stefano
^ permalink raw reply [flat|nested] 21+ messages in thread* Re: [PATCH v2 2/9] add errexit() to log an error message and exit with a single call
2023-02-09 17:45 ` Stefano Brivio
@ 2023-02-13 3:22 ` David Gibson
2023-02-13 10:46 ` Stefano Brivio
0 siblings, 1 reply; 21+ messages in thread
From: David Gibson @ 2023-02-13 3:22 UTC (permalink / raw)
To: Stefano Brivio; +Cc: Laine Stump, passt-dev, laine
[-- Attachment #1: Type: text/plain, Size: 2897 bytes --]
On Thu, Feb 09, 2023 at 06:45:32PM +0100, Stefano Brivio wrote:
> On Wed, 8 Feb 2023 12:48:31 -0500
> Laine Stump <laine@redhat.com> wrote:
>
> > Almost all occurences of err() are either immediately followed by
> > exit(EXIT_FAILURE), usage(argv[0]) (which itself then calls
> > exit(EXIT_FAILURE), or that is what's done immediately after returning
> > from the function that calls err(). Modify the errfn macro so that its
> > instantiations can include exit(EXIT_FAILURE) at the end, and use that
> > to create a new function errxit() that will log an error and then
> > exit.
> >
> > Signed-off-by: Laine Stump <laine@redhat.com>
> > ---
> > log.c | 13 ++++++++-----
> > log.h | 1 +
> > 2 files changed, 9 insertions(+), 5 deletions(-)
> >
> > diff --git a/log.c b/log.c
> > index 0ab0adf..4956914 100644
> > --- a/log.c
> > +++ b/log.c
> > @@ -45,7 +45,7 @@ static char log_header[BUFSIZ]; /* File header, written back on cuts */
> > static time_t log_start; /* Start timestamp */
> > int log_trace; /* --trace mode enabled */
> >
> > -#define logfn(name, level) \
> > +#define logfn(name, level, doexit) \
> > void name(const char *format, ...) { \
> > struct timespec tp; \
> > va_list args; \
> > @@ -76,6 +76,8 @@ void name(const char *format, ...) { \
> > if (format[strlen(format)] != '\n') \
> > fprintf(stderr, "\n"); \
> > } \
> > + if (doexit) \
>
> A blank line before this would make it more consistent.
>
> > + exit(EXIT_FAILURE); \
> > }
> >
> > /* Prefixes for log file messages, indexed by priority */
> > @@ -88,10 +90,11 @@ const char *logfile_prefix[] = {
> > " ", /* LOG_DEBUG */
> > };
> >
> > -logfn(err, LOG_ERR)
> > -logfn(warn, LOG_WARNING)
> > -logfn(info, LOG_INFO)
> > -logfn(debug, LOG_DEBUG)
> > +logfn(errexit, LOG_ERR, 1)
> > +logfn(err, LOG_ERR, 0)
> > +logfn(warn, LOG_WARNING, 0)
> > +logfn(info, LOG_INFO, 0)
> > +logfn(debug, LOG_DEBUG, 0)
> >
> > /**
> > * log_go_daemon() - tell logging subsystem that the process has been
> > diff --git a/log.h b/log.h
> > index a57c777..ed19415 100644
> > --- a/log.h
> > +++ b/log.h
> > @@ -10,6 +10,7 @@
> > #define LOGFILE_CUT_RATIO 30 /* When full, cut ~30% size */
> > #define LOGFILE_SIZE_MIN (5UL * MAX(BUFSIZ, PAGE_SIZE))
> >
> > +void errexit(const char *format, ...);
> > void err(const char *format, ...);
> > void warn(const char *format, ...);
> > void info(const char *format, ...);
>
> Other than that, this looks good to me.
LGTM. Personally I like to call such functions "die()".
--
David Gibson | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 21+ messages in thread* Re: [PATCH v2 2/9] add errexit() to log an error message and exit with a single call
2023-02-13 3:22 ` David Gibson
@ 2023-02-13 10:46 ` Stefano Brivio
0 siblings, 0 replies; 21+ messages in thread
From: Stefano Brivio @ 2023-02-13 10:46 UTC (permalink / raw)
To: David Gibson, Laine Stump; +Cc: passt-dev, laine
On Mon, 13 Feb 2023 14:22:33 +1100
David Gibson <david@gibson.dropbear.id.au> wrote:
> On Thu, Feb 09, 2023 at 06:45:32PM +0100, Stefano Brivio wrote:
> > On Wed, 8 Feb 2023 12:48:31 -0500
> > Laine Stump <laine@redhat.com> wrote:
> >
> > > Almost all occurences of err() are either immediately followed by
> > > exit(EXIT_FAILURE), usage(argv[0]) (which itself then calls
> > > exit(EXIT_FAILURE), or that is what's done immediately after returning
> > > from the function that calls err(). Modify the errfn macro so that its
> > > instantiations can include exit(EXIT_FAILURE) at the end, and use that
> > > to create a new function errxit() that will log an error and then
> > > exit.
> > >
> > > Signed-off-by: Laine Stump <laine@redhat.com>
> > > ---
> > > log.c | 13 ++++++++-----
> > > log.h | 1 +
> > > 2 files changed, 9 insertions(+), 5 deletions(-)
> > >
> > > diff --git a/log.c b/log.c
> > > index 0ab0adf..4956914 100644
> > > --- a/log.c
> > > +++ b/log.c
> > > @@ -45,7 +45,7 @@ static char log_header[BUFSIZ]; /* File header, written back on cuts */
> > > static time_t log_start; /* Start timestamp */
> > > int log_trace; /* --trace mode enabled */
> > >
> > > -#define logfn(name, level) \
> > > +#define logfn(name, level, doexit) \
> > > void name(const char *format, ...) { \
> > > struct timespec tp; \
> > > va_list args; \
> > > @@ -76,6 +76,8 @@ void name(const char *format, ...) { \
> > > if (format[strlen(format)] != '\n') \
> > > fprintf(stderr, "\n"); \
> > > } \
> > > + if (doexit) \
> >
> > A blank line before this would make it more consistent.
> >
> > > + exit(EXIT_FAILURE); \
> > > }
> > >
> > > /* Prefixes for log file messages, indexed by priority */
> > > @@ -88,10 +90,11 @@ const char *logfile_prefix[] = {
> > > " ", /* LOG_DEBUG */
> > > };
> > >
> > > -logfn(err, LOG_ERR)
> > > -logfn(warn, LOG_WARNING)
> > > -logfn(info, LOG_INFO)
> > > -logfn(debug, LOG_DEBUG)
> > > +logfn(errexit, LOG_ERR, 1)
> > > +logfn(err, LOG_ERR, 0)
> > > +logfn(warn, LOG_WARNING, 0)
> > > +logfn(info, LOG_INFO, 0)
> > > +logfn(debug, LOG_DEBUG, 0)
> > >
> > > /**
> > > * log_go_daemon() - tell logging subsystem that the process has been
> > > diff --git a/log.h b/log.h
> > > index a57c777..ed19415 100644
> > > --- a/log.h
> > > +++ b/log.h
> > > @@ -10,6 +10,7 @@
> > > #define LOGFILE_CUT_RATIO 30 /* When full, cut ~30% size */
> > > #define LOGFILE_SIZE_MIN (5UL * MAX(BUFSIZ, PAGE_SIZE))
> > >
> > > +void errexit(const char *format, ...);
> > > void err(const char *format, ...);
> > > void warn(const char *format, ...);
> > > void info(const char *format, ...);
> >
> > Other than that, this looks good to me.
>
> LGTM. Personally I like to call such functions "die()".
I was about to suggest that (it's shorter and conveys the same meaning),
then I thought die() would be a common library function. Actually, it's
not -- so I would also favour die() here.
--
Stefano
^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH v2 3/9] eliminate most calls to usage() in conf()
2023-02-08 17:48 [PATCH v2 0/9] error logging fixes Laine Stump
2023-02-08 17:48 ` [PATCH v2 1/9] log to stderr until process is daemonized, even if a logfile is set Laine Stump
2023-02-08 17:48 ` [PATCH v2 2/9] add errexit() to log an error message and exit with a single call Laine Stump
@ 2023-02-08 17:48 ` Laine Stump
2023-02-09 17:45 ` Stefano Brivio
2023-02-08 17:48 ` [PATCH v2 4/9] make conf_ports() exit immediately after logging error Laine Stump
` (5 subsequent siblings)
8 siblings, 1 reply; 21+ messages in thread
From: Laine Stump @ 2023-02-08 17:48 UTC (permalink / raw)
To: passt-dev, laine
Nearly all of the calls to usage() in conf() occur immediately after
logging a more detailed error message, and the fact that these errors
are occuring indicates that the user has already seen the passt usage
message (or read the manpage). Spamming the logfile with the complete
contents of the usage message serves only to obscure the more detailed
error message. The only time when the full usage message should be output
is if the user explicitly asks for it with -h (or its synonyms)
AS a start to eliminating the excessive calls to usage(), this patch
replaces most calls to err() followed by usage() with a call to
errexit() instead. A few other usage() calls remain, but their removal
involves bit more nuance that should be properly explained in
separate commit messages.
Signed-off-by: Laine Stump <laine@redhat.com>
---
conf.c | 339 +++++++++++++++++++++------------------------------------
1 file changed, 123 insertions(+), 216 deletions(-)
diff --git a/conf.c b/conf.c
index c37552d..e5035f7 100644
--- a/conf.c
+++ b/conf.c
@@ -1156,69 +1156,57 @@ void conf(struct ctx *c, int argc, char **argv)
case 0:
break;
case 2:
- if (c->mode != MODE_PASTA) {
- err("--userns is for pasta mode only");
- usage(argv[0]);
- }
+ if (c->mode != MODE_PASTA)
+ errexit("--userns is for pasta mode only");
ret = snprintf(userns, sizeof(userns), "%s", optarg);
- if (ret <= 0 || ret >= (int)sizeof(userns)) {
- err("Invalid userns: %s", optarg);
- usage(argv[0]);
- }
+ if (ret <= 0 || ret >= (int)sizeof(userns))
+ errexit("Invalid userns: %s", optarg);
+
break;
case 3:
- if (c->mode != MODE_PASTA) {
- err("--netns is for pasta mode only");
- usage(argv[0]);
- }
+ if (c->mode != MODE_PASTA)
+ errexit("--netns is for pasta mode only");
ret = conf_netns_opt(netns, optarg);
if (ret < 0)
usage(argv[0]);
break;
case 4:
- if (c->mode != MODE_PASTA) {
- err("--ns-mac-addr is for pasta mode only");
- usage(argv[0]);
- }
+ if (c->mode != MODE_PASTA)
+ errexit("--ns-mac-addr is for pasta mode only");
for (i = 0; i < ETH_ALEN; i++) {
errno = 0;
b = strtol(optarg + (intptr_t)i * 3, NULL, 16);
- if (b < 0 || b > UCHAR_MAX || errno) {
- err("Invalid MAC address: %s", optarg);
- usage(argv[0]);
- }
+ if (b < 0 || b > UCHAR_MAX || errno)
+ errexit("Invalid MAC address: %s", optarg);
+
c->mac_guest[i] = b;
}
break;
case 5:
- if (c->mode != MODE_PASTA) {
- err("--dhcp-dns is for pasta mode only");
- usage(argv[0]);
- }
+ if (c->mode != MODE_PASTA)
+ errexit("--dhcp-dns is for pasta mode only");
+
c->no_dhcp_dns = 0;
break;
case 6:
- if (c->mode != MODE_PASST) {
- err("--no-dhcp-dns is for passt mode only");
- usage(argv[0]);
- }
+ if (c->mode != MODE_PASST)
+ errexit("--no-dhcp-dns is for passt mode only");
+
c->no_dhcp_dns = 1;
break;
case 7:
- if (c->mode != MODE_PASTA) {
- err("--dhcp-search is for pasta mode only");
- usage(argv[0]);
- }
+ if (c->mode != MODE_PASTA)
+ errexit("--dhcp-search is for pasta mode only");
+
c->no_dhcp_dns_search = 0;
break;
case 8:
- if (c->mode != MODE_PASST) {
- err("--no-dhcp-search is for passt mode only");
- usage(argv[0]);
- }
+ if (c->mode != MODE_PASST)
+ errexit("--no-dhcp-search is for passt mode only");
+
c->no_dhcp_dns_search = 1;
break;
case 9:
@@ -1235,50 +1223,39 @@ void conf(struct ctx *c, int argc, char **argv)
!IN4_IS_ADDR_LOOPBACK(&c->ip4.dns_match))
break;
- err("Invalid DNS forwarding address: %s", optarg);
- usage(argv[0]);
+ errexit("Invalid DNS forwarding address: %s", optarg);
break;
case 10:
- if (c->mode != MODE_PASTA) {
- err("--no-netns-quit is for pasta mode only");
- usage(argv[0]);
- }
+ if (c->mode != MODE_PASTA)
+ errexit("--no-netns-quit is for pasta mode only");
+
c->no_netns_quit = 1;
break;
case 11:
- if (c->trace) {
- err("Multiple --trace options given");
- usage(argv[0]);
- }
+ if (c->trace)
+ errexit("Multiple --trace options given");
- if (c->quiet) {
- err("Either --trace or --quiet");
- usage(argv[0]);
- }
+ if (c->quiet)
+ errexit("Either --trace or --quiet");
c->trace = c->debug = 1;
break;
case 12:
- if (runas) {
- err("Multiple --runas options given");
- usage(argv[0]);
- }
+ if (runas)
+ errexit("Multiple --runas options given");
runas = optarg;
break;
case 13:
- if (logsize) {
- err("Multiple --log-size options given");
- usage(argv[0]);
- }
+ if (logsize)
+ errexit("Multiple --log-size options given");
errno = 0;
logsize = strtol(optarg, NULL, 0);
- if (logsize < LOGFILE_SIZE_MIN || errno) {
- err("Invalid --log-size: %s", optarg);
- usage(argv[0]);
- }
+ if (logsize < LOGFILE_SIZE_MIN || errno)
+ errexit("Invalid --log-size: %s", optarg);
+
break;
case 14:
fprintf(stdout,
@@ -1286,138 +1263,102 @@ void conf(struct ctx *c, int argc, char **argv)
fprintf(stdout, VERSION_BLOB);
exit(EXIT_SUCCESS);
case 'd':
- if (c->debug) {
- err("Multiple --debug options given");
- usage(argv[0]);
- }
+ if (c->debug)
+ errexit("Multiple --debug options given");
- if (c->quiet) {
- err("Either --debug or --quiet");
- usage(argv[0]);
- }
+ if (c->quiet)
+ errexit("Either --debug or --quiet");
c->debug = 1;
break;
case 'e':
- if (logfile) {
- err("Can't log to both file and stderr");
- usage(argv[0]);
- }
+ if (logfile)
+ errexit("Can't log to both file and stderr");
- if (c->stderr) {
- err("Multiple --stderr options given");
- usage(argv[0]);
- }
+ if (c->stderr)
+ errexit("Multiple --stderr options given");
c->stderr = 1;
break;
case 'l':
- if (c->stderr) {
- err("Can't log to both stderr and file");
- usage(argv[0]);
- }
+ if (c->stderr)
+ errexit("Can't log to both stderr and file");
- if (logfile) {
- err("Multiple --log-file options given");
- usage(argv[0]);
- }
+ if (logfile)
+ errexit("Multiple --log-file options given");
logfile = optarg;
break;
case 'q':
- if (c->quiet) {
- err("Multiple --quiet options given");
- usage(argv[0]);
- }
+ if (c->quiet)
+ errexit("Multiple --quiet options given");
- if (c->debug) {
- err("Either --debug or --quiet");
- usage(argv[0]);
- }
+ if (c->debug)
+ errexit("Either --debug or --quiet");
c->quiet = 1;
break;
case 'f':
- if (c->foreground) {
- err("Multiple --foreground options given");
- usage(argv[0]);
- }
+ if (c->foreground)
+ errexit("Multiple --foreground options given");
c->foreground = 1;
break;
case 's':
- if (*c->sock_path) {
- err("Multiple --socket options given");
- usage(argv[0]);
- }
+ if (*c->sock_path)
+ errexit("Multiple --socket options given");
ret = snprintf(c->sock_path, UNIX_SOCK_MAX - 1, "%s",
optarg);
- if (ret <= 0 || ret >= (int)sizeof(c->sock_path)) {
- err("Invalid socket path: %s", optarg);
- usage(argv[0]);
- }
+ if (ret <= 0 || ret >= (int)sizeof(c->sock_path))
+ errexit("Invalid socket path: %s", optarg);
+
break;
case 'F':
- if (c->fd_tap >= 0) {
- err("Multiple --fd options given");
- usage(argv[0]);
- }
+ if (c->fd_tap >= 0)
+ errexit("Multiple --fd options given");
errno = 0;
c->fd_tap = strtol(optarg, NULL, 0);
- if (c->fd_tap < 0 || errno) {
- err("Invalid --fd: %s", optarg);
- usage(argv[0]);
- }
+ if (c->fd_tap < 0 || errno)
+ errexit("Invalid --fd: %s", optarg);
c->one_off = true;
break;
case 'I':
- if (*c->pasta_ifn) {
- err("Multiple --ns-ifname options given");
- usage(argv[0]);
- }
+ if (*c->pasta_ifn)
+ errexit("Multiple --ns-ifname options given");
ret = snprintf(c->pasta_ifn, IFNAMSIZ - 1, "%s",
optarg);
- if (ret <= 0 || ret >= IFNAMSIZ - 1) {
- err("Invalid interface name: %s", optarg);
- usage(argv[0]);
- }
+ if (ret <= 0 || ret >= IFNAMSIZ - 1)
+ errexit("Invalid interface name: %s", optarg);
+
break;
case 'p':
- if (*c->pcap) {
- err("Multiple --pcap options given");
- usage(argv[0]);
- }
+ if (*c->pcap)
+ errexit("Multiple --pcap options given");
ret = snprintf(c->pcap, sizeof(c->pcap), "%s", optarg);
- if (ret <= 0 || ret >= (int)sizeof(c->pcap)) {
- err("Invalid pcap path: %s", optarg);
- usage(argv[0]);
- }
+ if (ret <= 0 || ret >= (int)sizeof(c->pcap))
+ errexit("Invalid pcap path: %s", optarg);
+
break;
case 'P':
- if (*c->pid_file) {
- err("Multiple --pid options given");
- usage(argv[0]);
- }
+ if (*c->pid_file)
+ errexit("Multiple --pid options given");
ret = snprintf(c->pid_file, sizeof(c->pid_file), "%s",
optarg);
- if (ret <= 0 || ret >= (int)sizeof(c->pid_file)) {
- err("Invalid PID file: %s", optarg);
- usage(argv[0]);
- }
+ if (ret <= 0 || ret >= (int)sizeof(c->pid_file))
+ errexit("Invalid PID file: %s", optarg);
+
break;
case 'm':
- if (c->mtu) {
- err("Multiple --mtu options given");
- usage(argv[0]);
- }
+ if (c->mtu)
+ errexit("Multiple --mtu options given");
errno = 0;
c->mtu = strtol(optarg, NULL, 0);
@@ -1427,11 +1368,9 @@ void conf(struct ctx *c, int argc, char **argv)
break;
}
- if (c->mtu < ETH_MIN_MTU || c->mtu > (int)ETH_MAX_MTU ||
- errno) {
- err("Invalid MTU: %s", optarg);
- usage(argv[0]);
- }
+ if (c->mtu < ETH_MIN_MTU || c->mtu > (int)ETH_MAX_MTU || errno)
+ errexit("Invalid MTU: %s", optarg);
+
break;
case 'a':
if (IN6_IS_ADDR_UNSPECIFIED(&c->ip6.addr) &&
@@ -1451,24 +1390,21 @@ void conf(struct ctx *c, int argc, char **argv)
!IN4_IS_ADDR_MULTICAST(&c->ip4.addr))
break;
- err("Invalid address: %s", optarg);
- usage(argv[0]);
+ errexit("Invalid address: %s", optarg);
break;
case 'n':
c->ip4.prefix_len = conf_ip4_prefix(optarg);
- if (c->ip4.prefix_len < 0) {
- err("Invalid netmask: %s", optarg);
- usage(argv[0]);
- }
+ if (c->ip4.prefix_len < 0)
+ errexit("Invalid netmask: %s", optarg);
+
break;
case 'M':
for (i = 0; i < ETH_ALEN; i++) {
errno = 0;
b = strtol(optarg + (intptr_t)i * 3, NULL, 16);
- if (b < 0 || b > UCHAR_MAX || errno) {
- err("Invalid MAC address: %s", optarg);
- usage(argv[0]);
- }
+ if (b < 0 || b > UCHAR_MAX || errno)
+ errexit("Invalid MAC address: %s", optarg);
+
c->mac[i] = b;
}
break;
@@ -1486,41 +1422,30 @@ void conf(struct ctx *c, int argc, char **argv)
!IN4_IS_ADDR_LOOPBACK(&c->ip4.gw))
break;
- err("Invalid gateway address: %s", optarg);
- usage(argv[0]);
+ errexit("Invalid gateway address: %s", optarg);
break;
case 'i':
- if (ifi) {
- err("Redundant interface: %s", optarg);
- usage(argv[0]);
- }
+ if (ifi)
+ errexit("Redundant interface: %s", optarg);
- if (!(ifi = if_nametoindex(optarg))) {
- err("Invalid interface name %s: %s", optarg,
- strerror(errno));
- usage(argv[0]);
- }
+ if (!(ifi = if_nametoindex(optarg)))
+ errexit("Invalid interface name %s: %s", optarg,
+ strerror(errno));
break;
case 'D':
if (!strcmp(optarg, "none")) {
- if (c->no_dns) {
- err("Redundant DNS options");
- usage(argv[0]);
- }
+ if (c->no_dns)
+ errexit("Redundant DNS options");
- if (dns4 - c->ip4.dns || dns6 - c->ip6.dns) {
- err("Conflicting DNS options");
- usage(argv[0]);
- }
+ if (dns4 - c->ip4.dns || dns6 - c->ip6.dns)
+ errexit("Conflicting DNS options");
c->no_dns = 1;
break;
}
- if (c->no_dns) {
- err("Conflicting DNS options");
- usage(argv[0]);
- }
+ if (c->no_dns)
+ errexit("Conflicting DNS options");
if (dns4 - &c->ip4.dns[0] < ARRAY_SIZE(c->ip4.dns) &&
inet_pton(AF_INET, optarg, dns4)) {
@@ -1534,29 +1459,22 @@ void conf(struct ctx *c, int argc, char **argv)
break;
}
- err("Cannot use DNS address %s", optarg);
- usage(argv[0]);
+ errexit("Cannot use DNS address %s", optarg);
break;
case 'S':
if (!strcmp(optarg, "none")) {
- if (c->no_dns_search) {
- err("Redundant DNS search options");
- usage(argv[0]);
- }
+ if (c->no_dns_search)
+ errexit("Redundant DNS search options");
- if (dnss != c->dns_search) {
- err("Conflicting DNS search options");
- usage(argv[0]);
- }
+ if (dnss != c->dns_search)
+ errexit("Conflicting DNS search options");
c->no_dns_search = 1;
break;
}
- if (c->no_dns_search) {
- err("Conflicting DNS search options");
- usage(argv[0]);
- }
+ if (c->no_dns_search)
+ errexit("Conflicting DNS search options");
if (dnss - c->dns_search < ARRAY_SIZE(c->dns_search)) {
ret = snprintf(dnss->n, sizeof(*c->dns_search),
@@ -1568,8 +1486,7 @@ void conf(struct ctx *c, int argc, char **argv)
break;
}
- err("Cannot use DNS search domain %s", optarg);
- usage(argv[0]);
+ errexit("Cannot use DNS search domain %s", optarg);
break;
case '4':
v4_only = true;
@@ -1578,15 +1495,11 @@ void conf(struct ctx *c, int argc, char **argv)
v6_only = true;
break;
case '1':
- if (c->mode != MODE_PASST) {
- err("--one-off is for passt mode only");
- usage(argv[0]);
- }
+ if (c->mode != MODE_PASST)
+ errexit("--one-off is for passt mode only");
- if (c->one_off) {
- err("Redundant --one-off option");
- usage(argv[0]);
- }
+ if (c->one_off)
+ errexit("Redundant --one-off option");
c->one_off = true;
break;
@@ -1604,15 +1517,11 @@ void conf(struct ctx *c, int argc, char **argv)
}
} while (name != -1);
- if (v4_only && v6_only) {
- err("Options ipv4-only and ipv6-only are mutually exclusive");
- usage(argv[0]);
- }
+ if (v4_only && v6_only)
+ errexit("Options ipv4-only and ipv6-only are mutually exclusive");
- if (*c->sock_path && c->fd_tap >= 0) {
- err("Options --socket and --fd are mutually exclusive");
- usage(argv[0]);
- }
+ if (*c->sock_path && c->fd_tap >= 0)
+ errexit("Options --socket and --fd are mutually exclusive");
ret = conf_ugid(runas, &uid, &gid);
if (ret)
@@ -1628,10 +1537,8 @@ void conf(struct ctx *c, int argc, char **argv)
c->ifi4 = conf_ip4(ifi, &c->ip4, c->mac);
if (!v4_only)
c->ifi6 = conf_ip6(ifi, &c->ip6, c->mac);
- if (!c->ifi4 && !c->ifi6) {
- err("External interface not usable");
- exit(EXIT_FAILURE);
- }
+ if (!c->ifi4 && !c->ifi6)
+ errexit("External interface not usable");
/* Inbound port options can be parsed now (after IPv4/IPv6 settings) */
optind = 1;
--
@@ -1156,69 +1156,57 @@ void conf(struct ctx *c, int argc, char **argv)
case 0:
break;
case 2:
- if (c->mode != MODE_PASTA) {
- err("--userns is for pasta mode only");
- usage(argv[0]);
- }
+ if (c->mode != MODE_PASTA)
+ errexit("--userns is for pasta mode only");
ret = snprintf(userns, sizeof(userns), "%s", optarg);
- if (ret <= 0 || ret >= (int)sizeof(userns)) {
- err("Invalid userns: %s", optarg);
- usage(argv[0]);
- }
+ if (ret <= 0 || ret >= (int)sizeof(userns))
+ errexit("Invalid userns: %s", optarg);
+
break;
case 3:
- if (c->mode != MODE_PASTA) {
- err("--netns is for pasta mode only");
- usage(argv[0]);
- }
+ if (c->mode != MODE_PASTA)
+ errexit("--netns is for pasta mode only");
ret = conf_netns_opt(netns, optarg);
if (ret < 0)
usage(argv[0]);
break;
case 4:
- if (c->mode != MODE_PASTA) {
- err("--ns-mac-addr is for pasta mode only");
- usage(argv[0]);
- }
+ if (c->mode != MODE_PASTA)
+ errexit("--ns-mac-addr is for pasta mode only");
for (i = 0; i < ETH_ALEN; i++) {
errno = 0;
b = strtol(optarg + (intptr_t)i * 3, NULL, 16);
- if (b < 0 || b > UCHAR_MAX || errno) {
- err("Invalid MAC address: %s", optarg);
- usage(argv[0]);
- }
+ if (b < 0 || b > UCHAR_MAX || errno)
+ errexit("Invalid MAC address: %s", optarg);
+
c->mac_guest[i] = b;
}
break;
case 5:
- if (c->mode != MODE_PASTA) {
- err("--dhcp-dns is for pasta mode only");
- usage(argv[0]);
- }
+ if (c->mode != MODE_PASTA)
+ errexit("--dhcp-dns is for pasta mode only");
+
c->no_dhcp_dns = 0;
break;
case 6:
- if (c->mode != MODE_PASST) {
- err("--no-dhcp-dns is for passt mode only");
- usage(argv[0]);
- }
+ if (c->mode != MODE_PASST)
+ errexit("--no-dhcp-dns is for passt mode only");
+
c->no_dhcp_dns = 1;
break;
case 7:
- if (c->mode != MODE_PASTA) {
- err("--dhcp-search is for pasta mode only");
- usage(argv[0]);
- }
+ if (c->mode != MODE_PASTA)
+ errexit("--dhcp-search is for pasta mode only");
+
c->no_dhcp_dns_search = 0;
break;
case 8:
- if (c->mode != MODE_PASST) {
- err("--no-dhcp-search is for passt mode only");
- usage(argv[0]);
- }
+ if (c->mode != MODE_PASST)
+ errexit("--no-dhcp-search is for passt mode only");
+
c->no_dhcp_dns_search = 1;
break;
case 9:
@@ -1235,50 +1223,39 @@ void conf(struct ctx *c, int argc, char **argv)
!IN4_IS_ADDR_LOOPBACK(&c->ip4.dns_match))
break;
- err("Invalid DNS forwarding address: %s", optarg);
- usage(argv[0]);
+ errexit("Invalid DNS forwarding address: %s", optarg);
break;
case 10:
- if (c->mode != MODE_PASTA) {
- err("--no-netns-quit is for pasta mode only");
- usage(argv[0]);
- }
+ if (c->mode != MODE_PASTA)
+ errexit("--no-netns-quit is for pasta mode only");
+
c->no_netns_quit = 1;
break;
case 11:
- if (c->trace) {
- err("Multiple --trace options given");
- usage(argv[0]);
- }
+ if (c->trace)
+ errexit("Multiple --trace options given");
- if (c->quiet) {
- err("Either --trace or --quiet");
- usage(argv[0]);
- }
+ if (c->quiet)
+ errexit("Either --trace or --quiet");
c->trace = c->debug = 1;
break;
case 12:
- if (runas) {
- err("Multiple --runas options given");
- usage(argv[0]);
- }
+ if (runas)
+ errexit("Multiple --runas options given");
runas = optarg;
break;
case 13:
- if (logsize) {
- err("Multiple --log-size options given");
- usage(argv[0]);
- }
+ if (logsize)
+ errexit("Multiple --log-size options given");
errno = 0;
logsize = strtol(optarg, NULL, 0);
- if (logsize < LOGFILE_SIZE_MIN || errno) {
- err("Invalid --log-size: %s", optarg);
- usage(argv[0]);
- }
+ if (logsize < LOGFILE_SIZE_MIN || errno)
+ errexit("Invalid --log-size: %s", optarg);
+
break;
case 14:
fprintf(stdout,
@@ -1286,138 +1263,102 @@ void conf(struct ctx *c, int argc, char **argv)
fprintf(stdout, VERSION_BLOB);
exit(EXIT_SUCCESS);
case 'd':
- if (c->debug) {
- err("Multiple --debug options given");
- usage(argv[0]);
- }
+ if (c->debug)
+ errexit("Multiple --debug options given");
- if (c->quiet) {
- err("Either --debug or --quiet");
- usage(argv[0]);
- }
+ if (c->quiet)
+ errexit("Either --debug or --quiet");
c->debug = 1;
break;
case 'e':
- if (logfile) {
- err("Can't log to both file and stderr");
- usage(argv[0]);
- }
+ if (logfile)
+ errexit("Can't log to both file and stderr");
- if (c->stderr) {
- err("Multiple --stderr options given");
- usage(argv[0]);
- }
+ if (c->stderr)
+ errexit("Multiple --stderr options given");
c->stderr = 1;
break;
case 'l':
- if (c->stderr) {
- err("Can't log to both stderr and file");
- usage(argv[0]);
- }
+ if (c->stderr)
+ errexit("Can't log to both stderr and file");
- if (logfile) {
- err("Multiple --log-file options given");
- usage(argv[0]);
- }
+ if (logfile)
+ errexit("Multiple --log-file options given");
logfile = optarg;
break;
case 'q':
- if (c->quiet) {
- err("Multiple --quiet options given");
- usage(argv[0]);
- }
+ if (c->quiet)
+ errexit("Multiple --quiet options given");
- if (c->debug) {
- err("Either --debug or --quiet");
- usage(argv[0]);
- }
+ if (c->debug)
+ errexit("Either --debug or --quiet");
c->quiet = 1;
break;
case 'f':
- if (c->foreground) {
- err("Multiple --foreground options given");
- usage(argv[0]);
- }
+ if (c->foreground)
+ errexit("Multiple --foreground options given");
c->foreground = 1;
break;
case 's':
- if (*c->sock_path) {
- err("Multiple --socket options given");
- usage(argv[0]);
- }
+ if (*c->sock_path)
+ errexit("Multiple --socket options given");
ret = snprintf(c->sock_path, UNIX_SOCK_MAX - 1, "%s",
optarg);
- if (ret <= 0 || ret >= (int)sizeof(c->sock_path)) {
- err("Invalid socket path: %s", optarg);
- usage(argv[0]);
- }
+ if (ret <= 0 || ret >= (int)sizeof(c->sock_path))
+ errexit("Invalid socket path: %s", optarg);
+
break;
case 'F':
- if (c->fd_tap >= 0) {
- err("Multiple --fd options given");
- usage(argv[0]);
- }
+ if (c->fd_tap >= 0)
+ errexit("Multiple --fd options given");
errno = 0;
c->fd_tap = strtol(optarg, NULL, 0);
- if (c->fd_tap < 0 || errno) {
- err("Invalid --fd: %s", optarg);
- usage(argv[0]);
- }
+ if (c->fd_tap < 0 || errno)
+ errexit("Invalid --fd: %s", optarg);
c->one_off = true;
break;
case 'I':
- if (*c->pasta_ifn) {
- err("Multiple --ns-ifname options given");
- usage(argv[0]);
- }
+ if (*c->pasta_ifn)
+ errexit("Multiple --ns-ifname options given");
ret = snprintf(c->pasta_ifn, IFNAMSIZ - 1, "%s",
optarg);
- if (ret <= 0 || ret >= IFNAMSIZ - 1) {
- err("Invalid interface name: %s", optarg);
- usage(argv[0]);
- }
+ if (ret <= 0 || ret >= IFNAMSIZ - 1)
+ errexit("Invalid interface name: %s", optarg);
+
break;
case 'p':
- if (*c->pcap) {
- err("Multiple --pcap options given");
- usage(argv[0]);
- }
+ if (*c->pcap)
+ errexit("Multiple --pcap options given");
ret = snprintf(c->pcap, sizeof(c->pcap), "%s", optarg);
- if (ret <= 0 || ret >= (int)sizeof(c->pcap)) {
- err("Invalid pcap path: %s", optarg);
- usage(argv[0]);
- }
+ if (ret <= 0 || ret >= (int)sizeof(c->pcap))
+ errexit("Invalid pcap path: %s", optarg);
+
break;
case 'P':
- if (*c->pid_file) {
- err("Multiple --pid options given");
- usage(argv[0]);
- }
+ if (*c->pid_file)
+ errexit("Multiple --pid options given");
ret = snprintf(c->pid_file, sizeof(c->pid_file), "%s",
optarg);
- if (ret <= 0 || ret >= (int)sizeof(c->pid_file)) {
- err("Invalid PID file: %s", optarg);
- usage(argv[0]);
- }
+ if (ret <= 0 || ret >= (int)sizeof(c->pid_file))
+ errexit("Invalid PID file: %s", optarg);
+
break;
case 'm':
- if (c->mtu) {
- err("Multiple --mtu options given");
- usage(argv[0]);
- }
+ if (c->mtu)
+ errexit("Multiple --mtu options given");
errno = 0;
c->mtu = strtol(optarg, NULL, 0);
@@ -1427,11 +1368,9 @@ void conf(struct ctx *c, int argc, char **argv)
break;
}
- if (c->mtu < ETH_MIN_MTU || c->mtu > (int)ETH_MAX_MTU ||
- errno) {
- err("Invalid MTU: %s", optarg);
- usage(argv[0]);
- }
+ if (c->mtu < ETH_MIN_MTU || c->mtu > (int)ETH_MAX_MTU || errno)
+ errexit("Invalid MTU: %s", optarg);
+
break;
case 'a':
if (IN6_IS_ADDR_UNSPECIFIED(&c->ip6.addr) &&
@@ -1451,24 +1390,21 @@ void conf(struct ctx *c, int argc, char **argv)
!IN4_IS_ADDR_MULTICAST(&c->ip4.addr))
break;
- err("Invalid address: %s", optarg);
- usage(argv[0]);
+ errexit("Invalid address: %s", optarg);
break;
case 'n':
c->ip4.prefix_len = conf_ip4_prefix(optarg);
- if (c->ip4.prefix_len < 0) {
- err("Invalid netmask: %s", optarg);
- usage(argv[0]);
- }
+ if (c->ip4.prefix_len < 0)
+ errexit("Invalid netmask: %s", optarg);
+
break;
case 'M':
for (i = 0; i < ETH_ALEN; i++) {
errno = 0;
b = strtol(optarg + (intptr_t)i * 3, NULL, 16);
- if (b < 0 || b > UCHAR_MAX || errno) {
- err("Invalid MAC address: %s", optarg);
- usage(argv[0]);
- }
+ if (b < 0 || b > UCHAR_MAX || errno)
+ errexit("Invalid MAC address: %s", optarg);
+
c->mac[i] = b;
}
break;
@@ -1486,41 +1422,30 @@ void conf(struct ctx *c, int argc, char **argv)
!IN4_IS_ADDR_LOOPBACK(&c->ip4.gw))
break;
- err("Invalid gateway address: %s", optarg);
- usage(argv[0]);
+ errexit("Invalid gateway address: %s", optarg);
break;
case 'i':
- if (ifi) {
- err("Redundant interface: %s", optarg);
- usage(argv[0]);
- }
+ if (ifi)
+ errexit("Redundant interface: %s", optarg);
- if (!(ifi = if_nametoindex(optarg))) {
- err("Invalid interface name %s: %s", optarg,
- strerror(errno));
- usage(argv[0]);
- }
+ if (!(ifi = if_nametoindex(optarg)))
+ errexit("Invalid interface name %s: %s", optarg,
+ strerror(errno));
break;
case 'D':
if (!strcmp(optarg, "none")) {
- if (c->no_dns) {
- err("Redundant DNS options");
- usage(argv[0]);
- }
+ if (c->no_dns)
+ errexit("Redundant DNS options");
- if (dns4 - c->ip4.dns || dns6 - c->ip6.dns) {
- err("Conflicting DNS options");
- usage(argv[0]);
- }
+ if (dns4 - c->ip4.dns || dns6 - c->ip6.dns)
+ errexit("Conflicting DNS options");
c->no_dns = 1;
break;
}
- if (c->no_dns) {
- err("Conflicting DNS options");
- usage(argv[0]);
- }
+ if (c->no_dns)
+ errexit("Conflicting DNS options");
if (dns4 - &c->ip4.dns[0] < ARRAY_SIZE(c->ip4.dns) &&
inet_pton(AF_INET, optarg, dns4)) {
@@ -1534,29 +1459,22 @@ void conf(struct ctx *c, int argc, char **argv)
break;
}
- err("Cannot use DNS address %s", optarg);
- usage(argv[0]);
+ errexit("Cannot use DNS address %s", optarg);
break;
case 'S':
if (!strcmp(optarg, "none")) {
- if (c->no_dns_search) {
- err("Redundant DNS search options");
- usage(argv[0]);
- }
+ if (c->no_dns_search)
+ errexit("Redundant DNS search options");
- if (dnss != c->dns_search) {
- err("Conflicting DNS search options");
- usage(argv[0]);
- }
+ if (dnss != c->dns_search)
+ errexit("Conflicting DNS search options");
c->no_dns_search = 1;
break;
}
- if (c->no_dns_search) {
- err("Conflicting DNS search options");
- usage(argv[0]);
- }
+ if (c->no_dns_search)
+ errexit("Conflicting DNS search options");
if (dnss - c->dns_search < ARRAY_SIZE(c->dns_search)) {
ret = snprintf(dnss->n, sizeof(*c->dns_search),
@@ -1568,8 +1486,7 @@ void conf(struct ctx *c, int argc, char **argv)
break;
}
- err("Cannot use DNS search domain %s", optarg);
- usage(argv[0]);
+ errexit("Cannot use DNS search domain %s", optarg);
break;
case '4':
v4_only = true;
@@ -1578,15 +1495,11 @@ void conf(struct ctx *c, int argc, char **argv)
v6_only = true;
break;
case '1':
- if (c->mode != MODE_PASST) {
- err("--one-off is for passt mode only");
- usage(argv[0]);
- }
+ if (c->mode != MODE_PASST)
+ errexit("--one-off is for passt mode only");
- if (c->one_off) {
- err("Redundant --one-off option");
- usage(argv[0]);
- }
+ if (c->one_off)
+ errexit("Redundant --one-off option");
c->one_off = true;
break;
@@ -1604,15 +1517,11 @@ void conf(struct ctx *c, int argc, char **argv)
}
} while (name != -1);
- if (v4_only && v6_only) {
- err("Options ipv4-only and ipv6-only are mutually exclusive");
- usage(argv[0]);
- }
+ if (v4_only && v6_only)
+ errexit("Options ipv4-only and ipv6-only are mutually exclusive");
- if (*c->sock_path && c->fd_tap >= 0) {
- err("Options --socket and --fd are mutually exclusive");
- usage(argv[0]);
- }
+ if (*c->sock_path && c->fd_tap >= 0)
+ errexit("Options --socket and --fd are mutually exclusive");
ret = conf_ugid(runas, &uid, &gid);
if (ret)
@@ -1628,10 +1537,8 @@ void conf(struct ctx *c, int argc, char **argv)
c->ifi4 = conf_ip4(ifi, &c->ip4, c->mac);
if (!v4_only)
c->ifi6 = conf_ip6(ifi, &c->ip6, c->mac);
- if (!c->ifi4 && !c->ifi6) {
- err("External interface not usable");
- exit(EXIT_FAILURE);
- }
+ if (!c->ifi4 && !c->ifi6)
+ errexit("External interface not usable");
/* Inbound port options can be parsed now (after IPv4/IPv6 settings) */
optind = 1;
--
2.39.1
^ permalink raw reply related [flat|nested] 21+ messages in thread* Re: [PATCH v2 3/9] eliminate most calls to usage() in conf()
2023-02-08 17:48 ` [PATCH v2 3/9] eliminate most calls to usage() in conf() Laine Stump
@ 2023-02-09 17:45 ` Stefano Brivio
0 siblings, 0 replies; 21+ messages in thread
From: Stefano Brivio @ 2023-02-09 17:45 UTC (permalink / raw)
To: Laine Stump; +Cc: passt-dev, laine
On Wed, 8 Feb 2023 12:48:32 -0500
Laine Stump <laine@redhat.com> wrote:
> Nearly all of the calls to usage() in conf() occur immediately after
> logging a more detailed error message, and the fact that these errors
> are occuring indicates that the user has already seen the passt usage
> message (or read the manpage). Spamming the logfile with the complete
> contents of the usage message serves only to obscure the more detailed
> error message. The only time when the full usage message should be output
> is if the user explicitly asks for it with -h (or its synonyms)
>
> AS a start to eliminating the excessive calls to usage(), this patch
> replaces most calls to err() followed by usage() with a call to
> errexit() instead. A few other usage() calls remain, but their removal
> involves bit more nuance that should be properly explained in
> separate commit messages.
This looks good to me, just a few nits:
> Signed-off-by: Laine Stump <laine@redhat.com>
> ---
> conf.c | 339 +++++++++++++++++++++------------------------------------
> 1 file changed, 123 insertions(+), 216 deletions(-)
>
> diff --git a/conf.c b/conf.c
> index c37552d..e5035f7 100644
> --- a/conf.c
> +++ b/conf.c
> @@ -1156,69 +1156,57 @@ void conf(struct ctx *c, int argc, char **argv)
> case 0:
> break;
> case 2:
> - if (c->mode != MODE_PASTA) {
> - err("--userns is for pasta mode only");
> - usage(argv[0]);
> - }
> + if (c->mode != MODE_PASTA)
> + errexit("--userns is for pasta mode only");
>
> ret = snprintf(userns, sizeof(userns), "%s", optarg);
> - if (ret <= 0 || ret >= (int)sizeof(userns)) {
> - err("Invalid userns: %s", optarg);
> - usage(argv[0]);
> - }
> + if (ret <= 0 || ret >= (int)sizeof(userns))
> + errexit("Invalid userns: %s", optarg);
> +
> break;
> case 3:
> - if (c->mode != MODE_PASTA) {
> - err("--netns is for pasta mode only");
> - usage(argv[0]);
> - }
> + if (c->mode != MODE_PASTA)
> + errexit("--netns is for pasta mode only");
>
> ret = conf_netns_opt(netns, optarg);
> if (ret < 0)
> usage(argv[0]);
> break;
> case 4:
> - if (c->mode != MODE_PASTA) {
> - err("--ns-mac-addr is for pasta mode only");
> - usage(argv[0]);
> - }
> + if (c->mode != MODE_PASTA)
> + errexit("--ns-mac-addr is for pasta mode only");
>
> for (i = 0; i < ETH_ALEN; i++) {
> errno = 0;
> b = strtol(optarg + (intptr_t)i * 3, NULL, 16);
> - if (b < 0 || b > UCHAR_MAX || errno) {
> - err("Invalid MAC address: %s", optarg);
> - usage(argv[0]);
> - }
> + if (b < 0 || b > UCHAR_MAX || errno)
> + errexit("Invalid MAC address: %s", optarg);
You should split the line before 'optarg'.
> +
> c->mac_guest[i] = b;
> }
> break;
> case 5:
> - if (c->mode != MODE_PASTA) {
> - err("--dhcp-dns is for pasta mode only");
> - usage(argv[0]);
> - }
> + if (c->mode != MODE_PASTA)
> + errexit("--dhcp-dns is for pasta mode only");
> +
> c->no_dhcp_dns = 0;
> break;
> case 6:
> - if (c->mode != MODE_PASST) {
> - err("--no-dhcp-dns is for passt mode only");
> - usage(argv[0]);
> - }
> + if (c->mode != MODE_PASST)
> + errexit("--no-dhcp-dns is for passt mode only");
> +
> c->no_dhcp_dns = 1;
> break;
> case 7:
> - if (c->mode != MODE_PASTA) {
> - err("--dhcp-search is for pasta mode only");
> - usage(argv[0]);
> - }
> + if (c->mode != MODE_PASTA)
> + errexit("--dhcp-search is for pasta mode only");
> +
> c->no_dhcp_dns_search = 0;
> break;
> case 8:
> - if (c->mode != MODE_PASST) {
> - err("--no-dhcp-search is for passt mode only");
> - usage(argv[0]);
> - }
> + if (c->mode != MODE_PASST)
> + errexit("--no-dhcp-search is for passt mode only");
> +
> c->no_dhcp_dns_search = 1;
> break;
> case 9:
> @@ -1235,50 +1223,39 @@ void conf(struct ctx *c, int argc, char **argv)
> !IN4_IS_ADDR_LOOPBACK(&c->ip4.dns_match))
> break;
>
> - err("Invalid DNS forwarding address: %s", optarg);
> - usage(argv[0]);
> + errexit("Invalid DNS forwarding address: %s", optarg);
> break;
> case 10:
> - if (c->mode != MODE_PASTA) {
> - err("--no-netns-quit is for pasta mode only");
> - usage(argv[0]);
> - }
> + if (c->mode != MODE_PASTA)
> + errexit("--no-netns-quit is for pasta mode only");
> +
> c->no_netns_quit = 1;
> break;
> case 11:
> - if (c->trace) {
> - err("Multiple --trace options given");
> - usage(argv[0]);
> - }
> + if (c->trace)
> + errexit("Multiple --trace options given");
>
> - if (c->quiet) {
> - err("Either --trace or --quiet");
> - usage(argv[0]);
> - }
> + if (c->quiet)
> + errexit("Either --trace or --quiet");
>
> c->trace = c->debug = 1;
> break;
> case 12:
> - if (runas) {
> - err("Multiple --runas options given");
> - usage(argv[0]);
> - }
> + if (runas)
> + errexit("Multiple --runas options given");
>
> runas = optarg;
> break;
> case 13:
> - if (logsize) {
> - err("Multiple --log-size options given");
> - usage(argv[0]);
> - }
> + if (logsize)
> + errexit("Multiple --log-size options given");
>
> errno = 0;
> logsize = strtol(optarg, NULL, 0);
>
> - if (logsize < LOGFILE_SIZE_MIN || errno) {
> - err("Invalid --log-size: %s", optarg);
> - usage(argv[0]);
> - }
> + if (logsize < LOGFILE_SIZE_MIN || errno)
> + errexit("Invalid --log-size: %s", optarg);
> +
> break;
> case 14:
> fprintf(stdout,
> @@ -1286,138 +1263,102 @@ void conf(struct ctx *c, int argc, char **argv)
> fprintf(stdout, VERSION_BLOB);
> exit(EXIT_SUCCESS);
> case 'd':
> - if (c->debug) {
> - err("Multiple --debug options given");
> - usage(argv[0]);
> - }
> + if (c->debug)
> + errexit("Multiple --debug options given");
>
> - if (c->quiet) {
> - err("Either --debug or --quiet");
> - usage(argv[0]);
> - }
> + if (c->quiet)
> + errexit("Either --debug or --quiet");
>
> c->debug = 1;
> break;
> case 'e':
> - if (logfile) {
> - err("Can't log to both file and stderr");
> - usage(argv[0]);
> - }
> + if (logfile)
> + errexit("Can't log to both file and stderr");
>
> - if (c->stderr) {
> - err("Multiple --stderr options given");
> - usage(argv[0]);
> - }
> + if (c->stderr)
> + errexit("Multiple --stderr options given");
>
> c->stderr = 1;
> break;
> case 'l':
> - if (c->stderr) {
> - err("Can't log to both stderr and file");
> - usage(argv[0]);
> - }
> + if (c->stderr)
> + errexit("Can't log to both stderr and file");
>
> - if (logfile) {
> - err("Multiple --log-file options given");
> - usage(argv[0]);
> - }
> + if (logfile)
> + errexit("Multiple --log-file options given");
>
> logfile = optarg;
> break;
> case 'q':
> - if (c->quiet) {
> - err("Multiple --quiet options given");
> - usage(argv[0]);
> - }
> + if (c->quiet)
> + errexit("Multiple --quiet options given");
>
> - if (c->debug) {
> - err("Either --debug or --quiet");
> - usage(argv[0]);
> - }
> + if (c->debug)
> + errexit("Either --debug or --quiet");
>
> c->quiet = 1;
> break;
> case 'f':
> - if (c->foreground) {
> - err("Multiple --foreground options given");
> - usage(argv[0]);
> - }
> + if (c->foreground)
> + errexit("Multiple --foreground options given");
>
> c->foreground = 1;
> break;
> case 's':
> - if (*c->sock_path) {
> - err("Multiple --socket options given");
> - usage(argv[0]);
> - }
> + if (*c->sock_path)
> + errexit("Multiple --socket options given");
>
> ret = snprintf(c->sock_path, UNIX_SOCK_MAX - 1, "%s",
> optarg);
> - if (ret <= 0 || ret >= (int)sizeof(c->sock_path)) {
> - err("Invalid socket path: %s", optarg);
> - usage(argv[0]);
> - }
> + if (ret <= 0 || ret >= (int)sizeof(c->sock_path))
> + errexit("Invalid socket path: %s", optarg);
> +
> break;
> case 'F':
> - if (c->fd_tap >= 0) {
> - err("Multiple --fd options given");
> - usage(argv[0]);
> - }
> + if (c->fd_tap >= 0)
> + errexit("Multiple --fd options given");
>
> errno = 0;
> c->fd_tap = strtol(optarg, NULL, 0);
>
> - if (c->fd_tap < 0 || errno) {
> - err("Invalid --fd: %s", optarg);
> - usage(argv[0]);
> - }
> + if (c->fd_tap < 0 || errno)
> + errexit("Invalid --fd: %s", optarg);
>
> c->one_off = true;
>
> break;
> case 'I':
> - if (*c->pasta_ifn) {
> - err("Multiple --ns-ifname options given");
> - usage(argv[0]);
> - }
> + if (*c->pasta_ifn)
> + errexit("Multiple --ns-ifname options given");
>
> ret = snprintf(c->pasta_ifn, IFNAMSIZ - 1, "%s",
> optarg);
> - if (ret <= 0 || ret >= IFNAMSIZ - 1) {
> - err("Invalid interface name: %s", optarg);
> - usage(argv[0]);
> - }
> + if (ret <= 0 || ret >= IFNAMSIZ - 1)
> + errexit("Invalid interface name: %s", optarg);
> +
> break;
> case 'p':
> - if (*c->pcap) {
> - err("Multiple --pcap options given");
> - usage(argv[0]);
> - }
> + if (*c->pcap)
> + errexit("Multiple --pcap options given");
>
> ret = snprintf(c->pcap, sizeof(c->pcap), "%s", optarg);
> - if (ret <= 0 || ret >= (int)sizeof(c->pcap)) {
> - err("Invalid pcap path: %s", optarg);
> - usage(argv[0]);
> - }
> + if (ret <= 0 || ret >= (int)sizeof(c->pcap))
> + errexit("Invalid pcap path: %s", optarg);
> +
> break;
> case 'P':
> - if (*c->pid_file) {
> - err("Multiple --pid options given");
> - usage(argv[0]);
> - }
> + if (*c->pid_file)
> + errexit("Multiple --pid options given");
>
> ret = snprintf(c->pid_file, sizeof(c->pid_file), "%s",
> optarg);
> - if (ret <= 0 || ret >= (int)sizeof(c->pid_file)) {
> - err("Invalid PID file: %s", optarg);
> - usage(argv[0]);
> - }
> + if (ret <= 0 || ret >= (int)sizeof(c->pid_file))
> + errexit("Invalid PID file: %s", optarg);
> +
> break;
> case 'm':
> - if (c->mtu) {
> - err("Multiple --mtu options given");
> - usage(argv[0]);
> - }
> + if (c->mtu)
> + errexit("Multiple --mtu options given");
>
> errno = 0;
> c->mtu = strtol(optarg, NULL, 0);
> @@ -1427,11 +1368,9 @@ void conf(struct ctx *c, int argc, char **argv)
> break;
> }
>
> - if (c->mtu < ETH_MIN_MTU || c->mtu > (int)ETH_MAX_MTU ||
> - errno) {
> - err("Invalid MTU: %s", optarg);
> - usage(argv[0]);
> - }
> + if (c->mtu < ETH_MIN_MTU || c->mtu > (int)ETH_MAX_MTU || errno)
This line should still be split like it was.
> + errexit("Invalid MTU: %s", optarg);
> +
> break;
> case 'a':
> if (IN6_IS_ADDR_UNSPECIFIED(&c->ip6.addr) &&
> @@ -1451,24 +1390,21 @@ void conf(struct ctx *c, int argc, char **argv)
> !IN4_IS_ADDR_MULTICAST(&c->ip4.addr))
> break;
>
> - err("Invalid address: %s", optarg);
> - usage(argv[0]);
> + errexit("Invalid address: %s", optarg);
> break;
> case 'n':
> c->ip4.prefix_len = conf_ip4_prefix(optarg);
> - if (c->ip4.prefix_len < 0) {
> - err("Invalid netmask: %s", optarg);
> - usage(argv[0]);
> - }
> + if (c->ip4.prefix_len < 0)
> + errexit("Invalid netmask: %s", optarg);
> +
> break;
> case 'M':
> for (i = 0; i < ETH_ALEN; i++) {
> errno = 0;
> b = strtol(optarg + (intptr_t)i * 3, NULL, 16);
> - if (b < 0 || b > UCHAR_MAX || errno) {
> - err("Invalid MAC address: %s", optarg);
> - usage(argv[0]);
> - }
> + if (b < 0 || b > UCHAR_MAX || errno)
> + errexit("Invalid MAC address: %s", optarg);
Split before 'optarg'.
> +
> c->mac[i] = b;
> }
> break;
> @@ -1486,41 +1422,30 @@ void conf(struct ctx *c, int argc, char **argv)
> !IN4_IS_ADDR_LOOPBACK(&c->ip4.gw))
> break;
>
> - err("Invalid gateway address: %s", optarg);
> - usage(argv[0]);
> + errexit("Invalid gateway address: %s", optarg);
> break;
> case 'i':
> - if (ifi) {
> - err("Redundant interface: %s", optarg);
> - usage(argv[0]);
> - }
> + if (ifi)
> + errexit("Redundant interface: %s", optarg);
>
> - if (!(ifi = if_nametoindex(optarg))) {
> - err("Invalid interface name %s: %s", optarg,
> - strerror(errno));
> - usage(argv[0]);
> - }
> + if (!(ifi = if_nametoindex(optarg)))
> + errexit("Invalid interface name %s: %s", optarg,
> + strerror(errno));
> break;
> case 'D':
> if (!strcmp(optarg, "none")) {
> - if (c->no_dns) {
> - err("Redundant DNS options");
> - usage(argv[0]);
> - }
> + if (c->no_dns)
> + errexit("Redundant DNS options");
>
> - if (dns4 - c->ip4.dns || dns6 - c->ip6.dns) {
> - err("Conflicting DNS options");
> - usage(argv[0]);
> - }
> + if (dns4 - c->ip4.dns || dns6 - c->ip6.dns)
> + errexit("Conflicting DNS options");
>
> c->no_dns = 1;
> break;
> }
>
> - if (c->no_dns) {
> - err("Conflicting DNS options");
> - usage(argv[0]);
> - }
> + if (c->no_dns)
> + errexit("Conflicting DNS options");
>
> if (dns4 - &c->ip4.dns[0] < ARRAY_SIZE(c->ip4.dns) &&
> inet_pton(AF_INET, optarg, dns4)) {
> @@ -1534,29 +1459,22 @@ void conf(struct ctx *c, int argc, char **argv)
> break;
> }
>
> - err("Cannot use DNS address %s", optarg);
> - usage(argv[0]);
> + errexit("Cannot use DNS address %s", optarg);
> break;
> case 'S':
> if (!strcmp(optarg, "none")) {
> - if (c->no_dns_search) {
> - err("Redundant DNS search options");
> - usage(argv[0]);
> - }
> + if (c->no_dns_search)
> + errexit("Redundant DNS search options");
>
> - if (dnss != c->dns_search) {
> - err("Conflicting DNS search options");
> - usage(argv[0]);
> - }
> + if (dnss != c->dns_search)
> + errexit("Conflicting DNS search options");
>
> c->no_dns_search = 1;
> break;
> }
>
> - if (c->no_dns_search) {
> - err("Conflicting DNS search options");
> - usage(argv[0]);
> - }
> + if (c->no_dns_search)
> + errexit("Conflicting DNS search options");
>
> if (dnss - c->dns_search < ARRAY_SIZE(c->dns_search)) {
> ret = snprintf(dnss->n, sizeof(*c->dns_search),
> @@ -1568,8 +1486,7 @@ void conf(struct ctx *c, int argc, char **argv)
> break;
> }
>
> - err("Cannot use DNS search domain %s", optarg);
> - usage(argv[0]);
> + errexit("Cannot use DNS search domain %s", optarg);
> break;
> case '4':
> v4_only = true;
> @@ -1578,15 +1495,11 @@ void conf(struct ctx *c, int argc, char **argv)
> v6_only = true;
> break;
> case '1':
> - if (c->mode != MODE_PASST) {
> - err("--one-off is for passt mode only");
> - usage(argv[0]);
> - }
> + if (c->mode != MODE_PASST)
> + errexit("--one-off is for passt mode only");
>
> - if (c->one_off) {
> - err("Redundant --one-off option");
> - usage(argv[0]);
> - }
> + if (c->one_off)
> + errexit("Redundant --one-off option");
>
> c->one_off = true;
> break;
> @@ -1604,15 +1517,11 @@ void conf(struct ctx *c, int argc, char **argv)
> }
> } while (name != -1);
>
> - if (v4_only && v6_only) {
> - err("Options ipv4-only and ipv6-only are mutually exclusive");
> - usage(argv[0]);
> - }
> + if (v4_only && v6_only)
> + errexit("Options ipv4-only and ipv6-only are mutually exclusive");
>
> - if (*c->sock_path && c->fd_tap >= 0) {
> - err("Options --socket and --fd are mutually exclusive");
> - usage(argv[0]);
> - }
> + if (*c->sock_path && c->fd_tap >= 0)
> + errexit("Options --socket and --fd are mutually exclusive");
>
> ret = conf_ugid(runas, &uid, &gid);
> if (ret)
> @@ -1628,10 +1537,8 @@ void conf(struct ctx *c, int argc, char **argv)
> c->ifi4 = conf_ip4(ifi, &c->ip4, c->mac);
> if (!v4_only)
> c->ifi6 = conf_ip6(ifi, &c->ip6, c->mac);
> - if (!c->ifi4 && !c->ifi6) {
> - err("External interface not usable");
> - exit(EXIT_FAILURE);
> - }
> + if (!c->ifi4 && !c->ifi6)
> + errexit("External interface not usable");
>
> /* Inbound port options can be parsed now (after IPv4/IPv6 settings) */
> optind = 1;
--
Stefano
^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH v2 4/9] make conf_ports() exit immediately after logging error
2023-02-08 17:48 [PATCH v2 0/9] error logging fixes Laine Stump
` (2 preceding siblings ...)
2023-02-08 17:48 ` [PATCH v2 3/9] eliminate most calls to usage() in conf() Laine Stump
@ 2023-02-08 17:48 ` Laine Stump
2023-02-09 17:45 ` Stefano Brivio
2023-02-08 17:48 ` [PATCH v2 5/9] make conf_pasta_ns() " Laine Stump
` (4 subsequent siblings)
8 siblings, 1 reply; 21+ messages in thread
From: Laine Stump @ 2023-02-08 17:48 UTC (permalink / raw)
To: passt-dev, laine
Rather than having conf_ports() (possibly) log an error, and then let
the caller log the entire usage() message and exit, just log the error
and exit immediately.
For some errors, conf_ports would previously not log any specific
message, leaving it up to the user to determine the problem by
guessing. We replace all of those silent returns with errexit()
(logging a specific error), thus permitting us to make conf_ports()
return void, which simplifies the caller.
We can further simplify the two callers to conf_ports by moving the
check for a missing argument to the port options into conf_ports
itself, and make it more useful by again logging an informative error
for missing option instead of the generic usage() message.
Signed-off-by: Laine Stump <laine@redhat.com>
---
conf.c | 55 +++++++++++++++++++++++++++++++------------------------
1 file changed, 31 insertions(+), 24 deletions(-)
diff --git a/conf.c b/conf.c
index e5035f7..799b9ff 100644
--- a/conf.c
+++ b/conf.c
@@ -173,11 +173,9 @@ static int parse_port_range(const char *s, char **endptr,
* @optname: Short option name, t, T, u, or U
* @optarg: Option argument (port specification)
* @fwd: Pointer to @port_fwd to be updated
- *
- * Return: -EINVAL on parsing error, 0 otherwise
*/
-static int conf_ports(const struct ctx *c, char optname, const char *optarg,
- struct port_fwd *fwd)
+static void conf_ports(const struct ctx *c, char optname, const char *optarg,
+ struct port_fwd *fwd)
{
char addr_buf[sizeof(struct in6_addr)] = { 0 }, *addr = addr_buf;
char buf[BUFSIZ], *spec, *ifname = NULL, *p;
@@ -185,25 +183,37 @@ static int conf_ports(const struct ctx *c, char optname, const char *optarg,
sa_family_t af = AF_UNSPEC;
bool exclude_only = true;
+ if (!optarg)
+ errexit("missing argument to -%s option", optname);
+
if (!strcmp(optarg, "none")) {
if (fwd->mode)
- return -EINVAL;
+ goto mode_conflict;
+
fwd->mode = FWD_NONE;
- return 0;
+ return;
}
if (!strcmp(optarg, "auto")) {
- if (fwd->mode || c->mode != MODE_PASTA)
- return -EINVAL;
+ if (fwd->mode)
+ goto mode_conflict;
+
+ if (c->mode != MODE_PASTA)
+ errexit("'auto' port forwarding is only allowed for pasta");
+
fwd->mode = FWD_AUTO;
- return 0;
+ return;
}
if (!strcmp(optarg, "all")) {
unsigned i;
- if (fwd->mode || c->mode != MODE_PASST)
- return -EINVAL;
+ if (fwd->mode)
+ goto mode_conflict;
+
+ if (c->mode != MODE_PASST)
+ errexit("'all' port forwarding is only allowed for passt");
+
fwd->mode = FWD_ALL;
memset(fwd->map, 0xff, PORT_EPHEMERAL_MIN / 8);
@@ -214,11 +224,11 @@ static int conf_ports(const struct ctx *c, char optname, const char *optarg,
udp_sock_init(c, 0, AF_UNSPEC, NULL, NULL, i);
}
- return 0;
+ return;
}
if (fwd->mode > FWD_SPEC)
- return -EINVAL;
+ errexit("specific ports cannot be specified together with all/none/auto");
fwd->mode = FWD_SPEC;
@@ -292,7 +302,7 @@ static int conf_ports(const struct ctx *c, char optname, const char *optarg,
udp_sock_init(c, 0, af, addr, ifname, i);
}
- return 0;
+ return;
}
/* Now process base ranges, skipping exclusions */
@@ -339,14 +349,13 @@ static int conf_ports(const struct ctx *c, char optname, const char *optarg,
}
} while ((p = next_chunk(p, ',')));
- return 0;
+ return;
bad:
- err("Invalid port specifier %s", optarg);
- return -EINVAL;
-
+ errexit("Invalid port specifier %s", optarg);
overlap:
- err("Overlapping port specifier %s", optarg);
- return -EINVAL;
+ errexit("Overlapping port specifier %s", optarg);
+mode_conflict:
+ errexit("Port forwarding mode '%s' conflicts with previous mode", optarg);
}
/**
@@ -1549,8 +1558,7 @@ void conf(struct ctx *c, int argc, char **argv)
if ((name == 't' && (fwd = &c->tcp.fwd_in)) ||
(name == 'u' && (fwd = &c->udp.fwd_in.f))) {
- if (!optarg || conf_ports(c, name, optarg, fwd))
- usage(argv[0]);
+ conf_ports(c, name, optarg, fwd);
}
} while (name != -1);
@@ -1588,8 +1596,7 @@ void conf(struct ctx *c, int argc, char **argv)
if ((name == 'T' && (fwd = &c->tcp.fwd_out)) ||
(name == 'U' && (fwd = &c->udp.fwd_out.f))) {
- if (!optarg || conf_ports(c, name, optarg, fwd))
- usage(argv[0]);
+ conf_ports(c, name, optarg, fwd);
}
} while (name != -1);
--
@@ -173,11 +173,9 @@ static int parse_port_range(const char *s, char **endptr,
* @optname: Short option name, t, T, u, or U
* @optarg: Option argument (port specification)
* @fwd: Pointer to @port_fwd to be updated
- *
- * Return: -EINVAL on parsing error, 0 otherwise
*/
-static int conf_ports(const struct ctx *c, char optname, const char *optarg,
- struct port_fwd *fwd)
+static void conf_ports(const struct ctx *c, char optname, const char *optarg,
+ struct port_fwd *fwd)
{
char addr_buf[sizeof(struct in6_addr)] = { 0 }, *addr = addr_buf;
char buf[BUFSIZ], *spec, *ifname = NULL, *p;
@@ -185,25 +183,37 @@ static int conf_ports(const struct ctx *c, char optname, const char *optarg,
sa_family_t af = AF_UNSPEC;
bool exclude_only = true;
+ if (!optarg)
+ errexit("missing argument to -%s option", optname);
+
if (!strcmp(optarg, "none")) {
if (fwd->mode)
- return -EINVAL;
+ goto mode_conflict;
+
fwd->mode = FWD_NONE;
- return 0;
+ return;
}
if (!strcmp(optarg, "auto")) {
- if (fwd->mode || c->mode != MODE_PASTA)
- return -EINVAL;
+ if (fwd->mode)
+ goto mode_conflict;
+
+ if (c->mode != MODE_PASTA)
+ errexit("'auto' port forwarding is only allowed for pasta");
+
fwd->mode = FWD_AUTO;
- return 0;
+ return;
}
if (!strcmp(optarg, "all")) {
unsigned i;
- if (fwd->mode || c->mode != MODE_PASST)
- return -EINVAL;
+ if (fwd->mode)
+ goto mode_conflict;
+
+ if (c->mode != MODE_PASST)
+ errexit("'all' port forwarding is only allowed for passt");
+
fwd->mode = FWD_ALL;
memset(fwd->map, 0xff, PORT_EPHEMERAL_MIN / 8);
@@ -214,11 +224,11 @@ static int conf_ports(const struct ctx *c, char optname, const char *optarg,
udp_sock_init(c, 0, AF_UNSPEC, NULL, NULL, i);
}
- return 0;
+ return;
}
if (fwd->mode > FWD_SPEC)
- return -EINVAL;
+ errexit("specific ports cannot be specified together with all/none/auto");
fwd->mode = FWD_SPEC;
@@ -292,7 +302,7 @@ static int conf_ports(const struct ctx *c, char optname, const char *optarg,
udp_sock_init(c, 0, af, addr, ifname, i);
}
- return 0;
+ return;
}
/* Now process base ranges, skipping exclusions */
@@ -339,14 +349,13 @@ static int conf_ports(const struct ctx *c, char optname, const char *optarg,
}
} while ((p = next_chunk(p, ',')));
- return 0;
+ return;
bad:
- err("Invalid port specifier %s", optarg);
- return -EINVAL;
-
+ errexit("Invalid port specifier %s", optarg);
overlap:
- err("Overlapping port specifier %s", optarg);
- return -EINVAL;
+ errexit("Overlapping port specifier %s", optarg);
+mode_conflict:
+ errexit("Port forwarding mode '%s' conflicts with previous mode", optarg);
}
/**
@@ -1549,8 +1558,7 @@ void conf(struct ctx *c, int argc, char **argv)
if ((name == 't' && (fwd = &c->tcp.fwd_in)) ||
(name == 'u' && (fwd = &c->udp.fwd_in.f))) {
- if (!optarg || conf_ports(c, name, optarg, fwd))
- usage(argv[0]);
+ conf_ports(c, name, optarg, fwd);
}
} while (name != -1);
@@ -1588,8 +1596,7 @@ void conf(struct ctx *c, int argc, char **argv)
if ((name == 'T' && (fwd = &c->tcp.fwd_out)) ||
(name == 'U' && (fwd = &c->udp.fwd_out.f))) {
- if (!optarg || conf_ports(c, name, optarg, fwd))
- usage(argv[0]);
+ conf_ports(c, name, optarg, fwd);
}
} while (name != -1);
--
2.39.1
^ permalink raw reply related [flat|nested] 21+ messages in thread* Re: [PATCH v2 4/9] make conf_ports() exit immediately after logging error
2023-02-08 17:48 ` [PATCH v2 4/9] make conf_ports() exit immediately after logging error Laine Stump
@ 2023-02-09 17:45 ` Stefano Brivio
0 siblings, 0 replies; 21+ messages in thread
From: Stefano Brivio @ 2023-02-09 17:45 UTC (permalink / raw)
To: Laine Stump; +Cc: passt-dev, laine
On Wed, 8 Feb 2023 12:48:33 -0500
Laine Stump <laine@redhat.com> wrote:
> Rather than having conf_ports() (possibly) log an error, and then let
> the caller log the entire usage() message and exit, just log the error
> and exit immediately.
>
> For some errors, conf_ports would previously not log any specific
> message, leaving it up to the user to determine the problem by
> guessing. We replace all of those silent returns with errexit()
> (logging a specific error), thus permitting us to make conf_ports()
> return void, which simplifies the caller.
>
> We can further simplify the two callers to conf_ports by moving the
> check for a missing argument to the port options into conf_ports
> itself, and make it more useful by again logging an informative error
> for missing option instead of the generic usage() message.
>
> Signed-off-by: Laine Stump <laine@redhat.com>
> ---
> conf.c | 55 +++++++++++++++++++++++++++++++------------------------
> 1 file changed, 31 insertions(+), 24 deletions(-)
>
> diff --git a/conf.c b/conf.c
> index e5035f7..799b9ff 100644
> --- a/conf.c
> +++ b/conf.c
> @@ -173,11 +173,9 @@ static int parse_port_range(const char *s, char **endptr,
> * @optname: Short option name, t, T, u, or U
> * @optarg: Option argument (port specification)
> * @fwd: Pointer to @port_fwd to be updated
> - *
> - * Return: -EINVAL on parsing error, 0 otherwise
> */
> -static int conf_ports(const struct ctx *c, char optname, const char *optarg,
> - struct port_fwd *fwd)
> +static void conf_ports(const struct ctx *c, char optname, const char *optarg,
> + struct port_fwd *fwd)
> {
> char addr_buf[sizeof(struct in6_addr)] = { 0 }, *addr = addr_buf;
> char buf[BUFSIZ], *spec, *ifname = NULL, *p;
> @@ -185,25 +183,37 @@ static int conf_ports(const struct ctx *c, char optname, const char *optarg,
> sa_family_t af = AF_UNSPEC;
> bool exclude_only = true;
>
> + if (!optarg)
> + errexit("missing argument to -%s option", optname);
This can't happen, as all the options relevant to this function are
passed to getopt_long() with 'required_argument'. I see there's a
check on !optarg in the caller, but I think you can skip it.
> +
> if (!strcmp(optarg, "none")) {
> if (fwd->mode)
> - return -EINVAL;
> + goto mode_conflict;
> +
> fwd->mode = FWD_NONE;
> - return 0;
> + return;
> }
>
> if (!strcmp(optarg, "auto")) {
> - if (fwd->mode || c->mode != MODE_PASTA)
> - return -EINVAL;
> + if (fwd->mode)
> + goto mode_conflict;
> +
> + if (c->mode != MODE_PASTA)
> + errexit("'auto' port forwarding is only allowed for pasta");
> +
> fwd->mode = FWD_AUTO;
> - return 0;
> + return;
> }
>
> if (!strcmp(optarg, "all")) {
> unsigned i;
>
> - if (fwd->mode || c->mode != MODE_PASST)
> - return -EINVAL;
> + if (fwd->mode)
> + goto mode_conflict;
> +
> + if (c->mode != MODE_PASST)
> + errexit("'all' port forwarding is only allowed for passt");
> +
> fwd->mode = FWD_ALL;
> memset(fwd->map, 0xff, PORT_EPHEMERAL_MIN / 8);
>
> @@ -214,11 +224,11 @@ static int conf_ports(const struct ctx *c, char optname, const char *optarg,
> udp_sock_init(c, 0, AF_UNSPEC, NULL, NULL, i);
> }
>
> - return 0;
> + return;
> }
>
> if (fwd->mode > FWD_SPEC)
> - return -EINVAL;
> + errexit("specific ports cannot be specified together with all/none/auto");
s/specific/Specific/, for consistency.
>
> fwd->mode = FWD_SPEC;
>
> @@ -292,7 +302,7 @@ static int conf_ports(const struct ctx *c, char optname, const char *optarg,
> udp_sock_init(c, 0, af, addr, ifname, i);
> }
>
> - return 0;
> + return;
> }
>
> /* Now process base ranges, skipping exclusions */
> @@ -339,14 +349,13 @@ static int conf_ports(const struct ctx *c, char optname, const char *optarg,
> }
> } while ((p = next_chunk(p, ',')));
>
> - return 0;
> + return;
> bad:
> - err("Invalid port specifier %s", optarg);
> - return -EINVAL;
> -
> + errexit("Invalid port specifier %s", optarg);
> overlap:
> - err("Overlapping port specifier %s", optarg);
> - return -EINVAL;
> + errexit("Overlapping port specifier %s", optarg);
> +mode_conflict:
> + errexit("Port forwarding mode '%s' conflicts with previous mode", optarg);
Split before 'optarg'.
> }
>
> /**
> @@ -1549,8 +1558,7 @@ void conf(struct ctx *c, int argc, char **argv)
>
> if ((name == 't' && (fwd = &c->tcp.fwd_in)) ||
> (name == 'u' && (fwd = &c->udp.fwd_in.f))) {
> - if (!optarg || conf_ports(c, name, optarg, fwd))
> - usage(argv[0]);
> + conf_ports(c, name, optarg, fwd);
> }
> } while (name != -1);
>
> @@ -1588,8 +1596,7 @@ void conf(struct ctx *c, int argc, char **argv)
>
> if ((name == 'T' && (fwd = &c->tcp.fwd_out)) ||
> (name == 'U' && (fwd = &c->udp.fwd_out.f))) {
> - if (!optarg || conf_ports(c, name, optarg, fwd))
> - usage(argv[0]);
> + conf_ports(c, name, optarg, fwd);
> }
> } while (name != -1);
>
--
Stefano
^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH v2 5/9] make conf_pasta_ns() exit immediately after logging error
2023-02-08 17:48 [PATCH v2 0/9] error logging fixes Laine Stump
` (3 preceding siblings ...)
2023-02-08 17:48 ` [PATCH v2 4/9] make conf_ports() exit immediately after logging error Laine Stump
@ 2023-02-08 17:48 ` Laine Stump
2023-02-09 17:45 ` Stefano Brivio
2023-02-08 17:48 ` [PATCH v2 6/9] make conf_ugid() " Laine Stump
` (3 subsequent siblings)
8 siblings, 1 reply; 21+ messages in thread
From: Laine Stump @ 2023-02-08 17:48 UTC (permalink / raw)
To: passt-dev, laine
As with conf_ports, this allows us to make the function return void.
Signed-off-by: Laine Stump <laine@redhat.com>
---
conf.c | 37 ++++++++++++-------------------------
1 file changed, 12 insertions(+), 25 deletions(-)
diff --git a/conf.c b/conf.c
index 799b9ff..1e9c6f6 100644
--- a/conf.c
+++ b/conf.c
@@ -500,21 +500,15 @@ static int conf_netns_opt(char *netns, const char *arg)
* @optind: Index of first non-option argument
* @argc: Number of arguments
* @argv: Command line arguments
- *
- * Return: 0 on success, negative error code otherwise
*/
-static int conf_pasta_ns(int *netns_only, char *userns, char *netns,
- int optind, int argc, char *argv[])
+static void conf_pasta_ns(int *netns_only, char *userns, char *netns,
+ int optind, int argc, char *argv[])
{
- if (*netns_only && *userns) {
- err("Both --userns and --netns-only given");
- return -EINVAL;
- }
+ if (*netns_only && *userns)
+ errexit("Both --userns and --netns-only given");
- if (*netns && optind != argc) {
- err("Both --netns and PID or command given");
- return -EINVAL;
- }
+ if (*netns && optind != argc)
+ errexit("Both --netns and PID or command given");
if (optind + 1 == argc) {
char *endptr;
@@ -523,23 +517,19 @@ static int conf_pasta_ns(int *netns_only, char *userns, char *netns,
pidval = strtol(argv[optind], &endptr, 10);
if (!*endptr) {
/* Looks like a pid */
- if (pidval < 0 || pidval > INT_MAX) {
- err("Invalid PID %s", argv[optind]);
- return -EINVAL;
- }
+ if (pidval < 0 || pidval > INT_MAX)
+ errexit("Invalid PID %s", argv[optind]);
snprintf(netns, PATH_MAX, "/proc/%ld/ns/net", pidval);
if (!*userns)
snprintf(userns, PATH_MAX, "/proc/%ld/ns/user",
- pidval);
+ pidval);
}
}
/* Attaching to a netns/PID, with no userns given */
if (*netns && !*userns)
*netns_only = 1;
-
- return 0;
}
/** conf_ip4_prefix() - Parse an IPv4 prefix length or netmask
@@ -1562,13 +1552,10 @@ void conf(struct ctx *c, int argc, char **argv)
}
} while (name != -1);
- if (c->mode == MODE_PASTA) {
- if (conf_pasta_ns(&netns_only, userns, netns,
- optind, argc, argv) < 0)
- usage(argv[0]);
- } else if (optind != argc) {
+ if (c->mode == MODE_PASTA)
+ conf_pasta_ns(&netns_only, userns, netns, optind, argc, argv);
+ else if (optind != argc)
usage(argv[0]);
- }
isolate_user(uid, gid, !netns_only, userns, c->mode);
--
@@ -500,21 +500,15 @@ static int conf_netns_opt(char *netns, const char *arg)
* @optind: Index of first non-option argument
* @argc: Number of arguments
* @argv: Command line arguments
- *
- * Return: 0 on success, negative error code otherwise
*/
-static int conf_pasta_ns(int *netns_only, char *userns, char *netns,
- int optind, int argc, char *argv[])
+static void conf_pasta_ns(int *netns_only, char *userns, char *netns,
+ int optind, int argc, char *argv[])
{
- if (*netns_only && *userns) {
- err("Both --userns and --netns-only given");
- return -EINVAL;
- }
+ if (*netns_only && *userns)
+ errexit("Both --userns and --netns-only given");
- if (*netns && optind != argc) {
- err("Both --netns and PID or command given");
- return -EINVAL;
- }
+ if (*netns && optind != argc)
+ errexit("Both --netns and PID or command given");
if (optind + 1 == argc) {
char *endptr;
@@ -523,23 +517,19 @@ static int conf_pasta_ns(int *netns_only, char *userns, char *netns,
pidval = strtol(argv[optind], &endptr, 10);
if (!*endptr) {
/* Looks like a pid */
- if (pidval < 0 || pidval > INT_MAX) {
- err("Invalid PID %s", argv[optind]);
- return -EINVAL;
- }
+ if (pidval < 0 || pidval > INT_MAX)
+ errexit("Invalid PID %s", argv[optind]);
snprintf(netns, PATH_MAX, "/proc/%ld/ns/net", pidval);
if (!*userns)
snprintf(userns, PATH_MAX, "/proc/%ld/ns/user",
- pidval);
+ pidval);
}
}
/* Attaching to a netns/PID, with no userns given */
if (*netns && !*userns)
*netns_only = 1;
-
- return 0;
}
/** conf_ip4_prefix() - Parse an IPv4 prefix length or netmask
@@ -1562,13 +1552,10 @@ void conf(struct ctx *c, int argc, char **argv)
}
} while (name != -1);
- if (c->mode == MODE_PASTA) {
- if (conf_pasta_ns(&netns_only, userns, netns,
- optind, argc, argv) < 0)
- usage(argv[0]);
- } else if (optind != argc) {
+ if (c->mode == MODE_PASTA)
+ conf_pasta_ns(&netns_only, userns, netns, optind, argc, argv);
+ else if (optind != argc)
usage(argv[0]);
- }
isolate_user(uid, gid, !netns_only, userns, c->mode);
--
2.39.1
^ permalink raw reply related [flat|nested] 21+ messages in thread* Re: [PATCH v2 5/9] make conf_pasta_ns() exit immediately after logging error
2023-02-08 17:48 ` [PATCH v2 5/9] make conf_pasta_ns() " Laine Stump
@ 2023-02-09 17:45 ` Stefano Brivio
0 siblings, 0 replies; 21+ messages in thread
From: Stefano Brivio @ 2023-02-09 17:45 UTC (permalink / raw)
To: Laine Stump; +Cc: passt-dev, laine
On Wed, 8 Feb 2023 12:48:34 -0500
Laine Stump <laine@redhat.com> wrote:
> As with conf_ports, this allows us to make the function return void.
>
> Signed-off-by: Laine Stump <laine@redhat.com>
> ---
> conf.c | 37 ++++++++++++-------------------------
> 1 file changed, 12 insertions(+), 25 deletions(-)
>
> diff --git a/conf.c b/conf.c
> index 799b9ff..1e9c6f6 100644
> --- a/conf.c
> +++ b/conf.c
> @@ -500,21 +500,15 @@ static int conf_netns_opt(char *netns, const char *arg)
> * @optind: Index of first non-option argument
> * @argc: Number of arguments
> * @argv: Command line arguments
> - *
> - * Return: 0 on success, negative error code otherwise
> */
> -static int conf_pasta_ns(int *netns_only, char *userns, char *netns,
> - int optind, int argc, char *argv[])
> +static void conf_pasta_ns(int *netns_only, char *userns, char *netns,
> + int optind, int argc, char *argv[])
> {
> - if (*netns_only && *userns) {
> - err("Both --userns and --netns-only given");
> - return -EINVAL;
> - }
> + if (*netns_only && *userns)
> + errexit("Both --userns and --netns-only given");
>
> - if (*netns && optind != argc) {
> - err("Both --netns and PID or command given");
> - return -EINVAL;
> - }
> + if (*netns && optind != argc)
> + errexit("Both --netns and PID or command given");
>
> if (optind + 1 == argc) {
> char *endptr;
> @@ -523,23 +517,19 @@ static int conf_pasta_ns(int *netns_only, char *userns, char *netns,
> pidval = strtol(argv[optind], &endptr, 10);
> if (!*endptr) {
> /* Looks like a pid */
> - if (pidval < 0 || pidval > INT_MAX) {
> - err("Invalid PID %s", argv[optind]);
> - return -EINVAL;
> - }
> + if (pidval < 0 || pidval > INT_MAX)
> + errexit("Invalid PID %s", argv[optind]);
>
> snprintf(netns, PATH_MAX, "/proc/%ld/ns/net", pidval);
> if (!*userns)
> snprintf(userns, PATH_MAX, "/proc/%ld/ns/user",
> - pidval);
> + pidval);
Stray change.
> }
> }
>
> /* Attaching to a netns/PID, with no userns given */
> if (*netns && !*userns)
> *netns_only = 1;
> -
> - return 0;
> }
>
> /** conf_ip4_prefix() - Parse an IPv4 prefix length or netmask
> @@ -1562,13 +1552,10 @@ void conf(struct ctx *c, int argc, char **argv)
> }
> } while (name != -1);
>
> - if (c->mode == MODE_PASTA) {
> - if (conf_pasta_ns(&netns_only, userns, netns,
> - optind, argc, argv) < 0)
> - usage(argv[0]);
> - } else if (optind != argc) {
> + if (c->mode == MODE_PASTA)
> + conf_pasta_ns(&netns_only, userns, netns, optind, argc, argv);
> + else if (optind != argc)
> usage(argv[0]);
> - }
>
> isolate_user(uid, gid, !netns_only, userns, c->mode);
>
--
Stefano
^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH v2 6/9] make conf_ugid() exit immediately after logging error
2023-02-08 17:48 [PATCH v2 0/9] error logging fixes Laine Stump
` (4 preceding siblings ...)
2023-02-08 17:48 ` [PATCH v2 5/9] make conf_pasta_ns() " Laine Stump
@ 2023-02-08 17:48 ` Laine Stump
2023-02-13 4:23 ` Laine Stump
2023-02-08 17:48 ` [PATCH v2 7/9] make conf_netns_opt() " Laine Stump
` (2 subsequent siblings)
8 siblings, 1 reply; 21+ messages in thread
From: Laine Stump @ 2023-02-08 17:48 UTC (permalink / raw)
To: passt-dev, laine
Again, it can then be made to return void, simplifying the caller.
Signed-off-by: Laine Stump <laine@redhat.com>
---
conf.c | 22 +++++++---------------
1 file changed, 7 insertions(+), 15 deletions(-)
diff --git a/conf.c b/conf.c
index 1e9c6f6..5e9a6f9 100644
--- a/conf.c
+++ b/conf.c
@@ -998,10 +998,8 @@ static int conf_runas(char *opt, unsigned int *uid, unsigned int *gid)
* @runas: --runas option, may be NULL
* @uid: User ID, set on success
* @gid: Group ID, set on success
- *
- * Return: 0 on success, negative error code on failure
*/
-static int conf_ugid(char *runas, uid_t *uid, gid_t *gid)
+static void conf_ugid(char *runas, uid_t *uid, gid_t *gid)
{
const char root_uid_map[] = " 0 0 4294967295";
char buf[BUFSIZ];
@@ -1012,8 +1010,7 @@ static int conf_ugid(char *runas, uid_t *uid, gid_t *gid)
if (runas) {
ret = conf_runas(runas, uid, gid);
if (ret)
- err("Invalid --runas option: %s", runas);
- return ret;
+ errexit("Invalid --runas option: %s", runas);
}
/* ...otherwise default to current user and group... */
@@ -1022,20 +1019,18 @@ static int conf_ugid(char *runas, uid_t *uid, gid_t *gid)
/* ...as long as it's not root... */
if (*uid)
- return 0;
+ return;
/* ...or at least not root in the init namespace... */
if ((fd = open("/proc/self/uid_map", O_RDONLY | O_CLOEXEC)) < 0) {
- ret = -errno;
- err("Can't determine if we're in init namespace: %s",
- strerror(-ret));
- return ret;
+ errexit("Can't determine if we're in init namespace: %s",
+ strerror(-errno));
}
if (read(fd, buf, BUFSIZ) != sizeof(root_uid_map) ||
strncmp(buf, root_uid_map, sizeof(root_uid_map) - 1)) {
close(fd);
- return 0;
+ return;
}
close(fd);
@@ -1059,7 +1054,6 @@ static int conf_ugid(char *runas, uid_t *uid, gid_t *gid)
*uid = *gid = 65534;
#endif
}
- return 0;
}
/**
@@ -1522,9 +1516,7 @@ void conf(struct ctx *c, int argc, char **argv)
if (*c->sock_path && c->fd_tap >= 0)
errexit("Options --socket and --fd are mutually exclusive");
- ret = conf_ugid(runas, &uid, &gid);
- if (ret)
- usage(argv[0]);
+ conf_ugid(runas, &uid, &gid);
if (logfile) {
logfile_init(c->mode == MODE_PASST ? "passt" : "pasta",
--
@@ -998,10 +998,8 @@ static int conf_runas(char *opt, unsigned int *uid, unsigned int *gid)
* @runas: --runas option, may be NULL
* @uid: User ID, set on success
* @gid: Group ID, set on success
- *
- * Return: 0 on success, negative error code on failure
*/
-static int conf_ugid(char *runas, uid_t *uid, gid_t *gid)
+static void conf_ugid(char *runas, uid_t *uid, gid_t *gid)
{
const char root_uid_map[] = " 0 0 4294967295";
char buf[BUFSIZ];
@@ -1012,8 +1010,7 @@ static int conf_ugid(char *runas, uid_t *uid, gid_t *gid)
if (runas) {
ret = conf_runas(runas, uid, gid);
if (ret)
- err("Invalid --runas option: %s", runas);
- return ret;
+ errexit("Invalid --runas option: %s", runas);
}
/* ...otherwise default to current user and group... */
@@ -1022,20 +1019,18 @@ static int conf_ugid(char *runas, uid_t *uid, gid_t *gid)
/* ...as long as it's not root... */
if (*uid)
- return 0;
+ return;
/* ...or at least not root in the init namespace... */
if ((fd = open("/proc/self/uid_map", O_RDONLY | O_CLOEXEC)) < 0) {
- ret = -errno;
- err("Can't determine if we're in init namespace: %s",
- strerror(-ret));
- return ret;
+ errexit("Can't determine if we're in init namespace: %s",
+ strerror(-errno));
}
if (read(fd, buf, BUFSIZ) != sizeof(root_uid_map) ||
strncmp(buf, root_uid_map, sizeof(root_uid_map) - 1)) {
close(fd);
- return 0;
+ return;
}
close(fd);
@@ -1059,7 +1054,6 @@ static int conf_ugid(char *runas, uid_t *uid, gid_t *gid)
*uid = *gid = 65534;
#endif
}
- return 0;
}
/**
@@ -1522,9 +1516,7 @@ void conf(struct ctx *c, int argc, char **argv)
if (*c->sock_path && c->fd_tap >= 0)
errexit("Options --socket and --fd are mutually exclusive");
- ret = conf_ugid(runas, &uid, &gid);
- if (ret)
- usage(argv[0]);
+ conf_ugid(runas, &uid, &gid);
if (logfile) {
logfile_init(c->mode == MODE_PASST ? "passt" : "pasta",
--
2.39.1
^ permalink raw reply related [flat|nested] 21+ messages in thread* Re: [PATCH v2 6/9] make conf_ugid() exit immediately after logging error
2023-02-08 17:48 ` [PATCH v2 6/9] make conf_ugid() " Laine Stump
@ 2023-02-13 4:23 ` Laine Stump
0 siblings, 0 replies; 21+ messages in thread
From: Laine Stump @ 2023-02-13 4:23 UTC (permalink / raw)
To: passt-dev
On 2/8/23 12:48 PM, Laine Stump wrote:
> Again, it can then be made to return void, simplifying the caller.
>
> Signed-off-by: Laine Stump <laine@redhat.com>
> ---
> conf.c | 22 +++++++---------------
> 1 file changed, 7 insertions(+), 15 deletions(-)
>
> diff --git a/conf.c b/conf.c
> index 1e9c6f6..5e9a6f9 100644
> --- a/conf.c
> +++ b/conf.c
> @@ -998,10 +998,8 @@ static int conf_runas(char *opt, unsigned int *uid, unsigned int *gid)
> * @runas: --runas option, may be NULL
> * @uid: User ID, set on success
> * @gid: Group ID, set on success
> - *
> - * Return: 0 on success, negative error code on failure
> */
> -static int conf_ugid(char *runas, uid_t *uid, gid_t *gid)
> +static void conf_ugid(char *runas, uid_t *uid, gid_t *gid)
> {
> const char root_uid_map[] = " 0 0 4294967295";
> char buf[BUFSIZ];
> @@ -1012,8 +1010,7 @@ static int conf_ugid(char *runas, uid_t *uid, gid_t *gid)
> if (runas) {
> ret = conf_runas(runas, uid, gid);
> if (ret)
> - err("Invalid --runas option: %s", runas);
> - return ret;
> + errexit("Invalid --runas option: %s", runas);
Noticed while reviewing my own patches in email - I was moving too
quick/tired and counted the err() inside the "if (ret)" together with
the subsequent return, when the return is actually outside of "if
(ret)". I'll fix that up before reposting (along with your other
suggestions).
> }
>
> /* ...otherwise default to current user and group... */
> @@ -1022,20 +1019,18 @@ static int conf_ugid(char *runas, uid_t *uid, gid_t *gid)
>
> /* ...as long as it's not root... */
> if (*uid)
> - return 0;
> + return;
>
> /* ...or at least not root in the init namespace... */
> if ((fd = open("/proc/self/uid_map", O_RDONLY | O_CLOEXEC)) < 0) {
> - ret = -errno;
> - err("Can't determine if we're in init namespace: %s",
> - strerror(-ret));
> - return ret;
> + errexit("Can't determine if we're in init namespace: %s",
> + strerror(-errno));
> }
>
> if (read(fd, buf, BUFSIZ) != sizeof(root_uid_map) ||
> strncmp(buf, root_uid_map, sizeof(root_uid_map) - 1)) {
> close(fd);
> - return 0;
> + return;
> }
>
> close(fd);
> @@ -1059,7 +1054,6 @@ static int conf_ugid(char *runas, uid_t *uid, gid_t *gid)
> *uid = *gid = 65534;
> #endif
> }
> - return 0;
> }
>
> /**
> @@ -1522,9 +1516,7 @@ void conf(struct ctx *c, int argc, char **argv)
> if (*c->sock_path && c->fd_tap >= 0)
> errexit("Options --socket and --fd are mutually exclusive");
>
> - ret = conf_ugid(runas, &uid, &gid);
> - if (ret)
> - usage(argv[0]);
> + conf_ugid(runas, &uid, &gid);
>
> if (logfile) {
> logfile_init(c->mode == MODE_PASST ? "passt" : "pasta",
^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH v2 7/9] make conf_netns_opt() exit immediately after logging error
2023-02-08 17:48 [PATCH v2 0/9] error logging fixes Laine Stump
` (5 preceding siblings ...)
2023-02-08 17:48 ` [PATCH v2 6/9] make conf_ugid() " Laine Stump
@ 2023-02-08 17:48 ` Laine Stump
2023-02-08 17:48 ` [PATCH v2 8/9] log a detailed error (not usage()) when there are extra non-option arguments Laine Stump
2023-02-08 17:48 ` [PATCH v2 9/9] convert all remaining err() followed by exit() to errexit() Laine Stump
8 siblings, 0 replies; 21+ messages in thread
From: Laine Stump @ 2023-02-08 17:48 UTC (permalink / raw)
To: passt-dev, laine
...and return void to simplify the caller.
Signed-off-by: Laine Stump <laine@redhat.com>
---
conf.c | 16 ++++------------
1 file changed, 4 insertions(+), 12 deletions(-)
diff --git a/conf.c b/conf.c
index 5e9a6f9..1bfbc55 100644
--- a/conf.c
+++ b/conf.c
@@ -467,10 +467,8 @@ out:
* conf_netns_opt() - Parse --netns option
* @netns: buffer of size PATH_MAX, updated with netns path
* @arg: --netns argument
- *
- * Return: 0 on success, negative error code otherwise
*/
-static int conf_netns_opt(char *netns, const char *arg)
+static void conf_netns_opt(char *netns, const char *arg)
{
int ret;
@@ -482,12 +480,8 @@ static int conf_netns_opt(char *netns, const char *arg)
ret = snprintf(netns, PATH_MAX, "%s", arg);
}
- if (ret <= 0 || ret > PATH_MAX) {
- err("Network namespace name/path %s too long");
- return -E2BIG;
- }
-
- return 0;
+ if (ret <= 0 || ret > PATH_MAX)
+ errexit("Network namespace name/path %s too long");
}
/**
@@ -1161,9 +1155,7 @@ void conf(struct ctx *c, int argc, char **argv)
if (c->mode != MODE_PASTA)
errexit("--netns is for pasta mode only");
- ret = conf_netns_opt(netns, optarg);
- if (ret < 0)
- usage(argv[0]);
+ conf_netns_opt(netns, optarg);
break;
case 4:
if (c->mode != MODE_PASTA)
--
@@ -467,10 +467,8 @@ out:
* conf_netns_opt() - Parse --netns option
* @netns: buffer of size PATH_MAX, updated with netns path
* @arg: --netns argument
- *
- * Return: 0 on success, negative error code otherwise
*/
-static int conf_netns_opt(char *netns, const char *arg)
+static void conf_netns_opt(char *netns, const char *arg)
{
int ret;
@@ -482,12 +480,8 @@ static int conf_netns_opt(char *netns, const char *arg)
ret = snprintf(netns, PATH_MAX, "%s", arg);
}
- if (ret <= 0 || ret > PATH_MAX) {
- err("Network namespace name/path %s too long");
- return -E2BIG;
- }
-
- return 0;
+ if (ret <= 0 || ret > PATH_MAX)
+ errexit("Network namespace name/path %s too long");
}
/**
@@ -1161,9 +1155,7 @@ void conf(struct ctx *c, int argc, char **argv)
if (c->mode != MODE_PASTA)
errexit("--netns is for pasta mode only");
- ret = conf_netns_opt(netns, optarg);
- if (ret < 0)
- usage(argv[0]);
+ conf_netns_opt(netns, optarg);
break;
case 4:
if (c->mode != MODE_PASTA)
--
2.39.1
^ permalink raw reply related [flat|nested] 21+ messages in thread* [PATCH v2 8/9] log a detailed error (not usage()) when there are extra non-option arguments
2023-02-08 17:48 [PATCH v2 0/9] error logging fixes Laine Stump
` (6 preceding siblings ...)
2023-02-08 17:48 ` [PATCH v2 7/9] make conf_netns_opt() " Laine Stump
@ 2023-02-08 17:48 ` Laine Stump
2023-02-09 17:45 ` Stefano Brivio
2023-02-08 17:48 ` [PATCH v2 9/9] convert all remaining err() followed by exit() to errexit() Laine Stump
8 siblings, 1 reply; 21+ messages in thread
From: Laine Stump @ 2023-02-08 17:48 UTC (permalink / raw)
To: passt-dev, laine
Telling the user "this bit is wrong" is more useful than telling them
"these are all the potential things that would be right; actual error
identification is left as an exercise for the reader".
Signed-off-by: Laine Stump <laine@redhat.com>
---
conf.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/conf.c b/conf.c
index 1bfbc55..ce4e3e5 100644
--- a/conf.c
+++ b/conf.c
@@ -1539,7 +1539,7 @@ void conf(struct ctx *c, int argc, char **argv)
if (c->mode == MODE_PASTA)
conf_pasta_ns(&netns_only, userns, netns, optind, argc, argv);
else if (optind != argc)
- usage(argv[0]);
+ errexit("Extra non-option argument: %s", argv[optind]);
isolate_user(uid, gid, !netns_only, userns, c->mode);
--
@@ -1539,7 +1539,7 @@ void conf(struct ctx *c, int argc, char **argv)
if (c->mode == MODE_PASTA)
conf_pasta_ns(&netns_only, userns, netns, optind, argc, argv);
else if (optind != argc)
- usage(argv[0]);
+ errexit("Extra non-option argument: %s", argv[optind]);
isolate_user(uid, gid, !netns_only, userns, c->mode);
--
2.39.1
^ permalink raw reply related [flat|nested] 21+ messages in thread* Re: [PATCH v2 8/9] log a detailed error (not usage()) when there are extra non-option arguments
2023-02-08 17:48 ` [PATCH v2 8/9] log a detailed error (not usage()) when there are extra non-option arguments Laine Stump
@ 2023-02-09 17:45 ` Stefano Brivio
0 siblings, 0 replies; 21+ messages in thread
From: Stefano Brivio @ 2023-02-09 17:45 UTC (permalink / raw)
To: Laine Stump; +Cc: passt-dev, laine
On Wed, 8 Feb 2023 12:48:37 -0500
Laine Stump <laine@redhat.com> wrote:
> Telling the user "this bit is wrong" is more useful than telling them
> "these are all the potential things that would be right; actual error
> identification is left as an exercise for the reader".
Ah, yes, I guess. :)
>
> Signed-off-by: Laine Stump <laine@redhat.com>
> ---
> conf.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/conf.c b/conf.c
> index 1bfbc55..ce4e3e5 100644
> --- a/conf.c
> +++ b/conf.c
> @@ -1539,7 +1539,7 @@ void conf(struct ctx *c, int argc, char **argv)
> if (c->mode == MODE_PASTA)
> conf_pasta_ns(&netns_only, userns, netns, optind, argc, argv);
> else if (optind != argc)
> - usage(argv[0]);
> + errexit("Extra non-option argument: %s", argv[optind]);
>
> isolate_user(uid, gid, !netns_only, userns, c->mode);
>
--
Stefano
^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH v2 9/9] convert all remaining err() followed by exit() to errexit()
2023-02-08 17:48 [PATCH v2 0/9] error logging fixes Laine Stump
` (7 preceding siblings ...)
2023-02-08 17:48 ` [PATCH v2 8/9] log a detailed error (not usage()) when there are extra non-option arguments Laine Stump
@ 2023-02-08 17:48 ` Laine Stump
2023-02-09 17:45 ` Stefano Brivio
8 siblings, 1 reply; 21+ messages in thread
From: Laine Stump @ 2023-02-08 17:48 UTC (permalink / raw)
To: passt-dev, laine
This actually leaves us with 0 uses of err(), but someone could want
to use it in the future, so may as well leave it around.
Signed-off-by: Laine Stump <laine@redhat.com>
---
isolation.c | 78 +++++++++++++++++++----------------------------------
log.c | 6 ++---
netlink.c | 3 +--
passt.c | 12 +++------
pasta.c | 21 ++++++---------
tap.c | 30 +++++++--------------
6 files changed, 53 insertions(+), 97 deletions(-)
diff --git a/isolation.c b/isolation.c
index 4e6637d..0f709c6 100644
--- a/isolation.c
+++ b/isolation.c
@@ -103,10 +103,8 @@ static void drop_caps_ep_except(uint64_t keep)
struct __user_cap_data_struct data[CAP_WORDS];
int i;
- if (syscall(SYS_capget, &hdr, data)) {
- err("Couldn't get current capabilities: %s", strerror(errno));
- exit(EXIT_FAILURE);
- }
+ if (syscall(SYS_capget, &hdr, data))
+ errexit("Couldn't get current capabilities: %s", strerror(errno));
for (i = 0; i < CAP_WORDS; i++) {
uint32_t mask = keep >> (32 * i);
@@ -115,10 +113,8 @@ static void drop_caps_ep_except(uint64_t keep)
data[i].permitted &= mask;
}
- if (syscall(SYS_capset, &hdr, data)) {
- err("Couldn't drop capabilities: %s", strerror(errno));
- exit(EXIT_FAILURE);
- }
+ if (syscall(SYS_capset, &hdr, data))
+ errexit("Couldn't drop capabilities: %s", strerror(errno));
}
/**
@@ -154,26 +150,20 @@ static void clamp_caps(void)
* normal operation, so carry on without it.
*/
if (prctl(PR_CAPBSET_DROP, i, 0, 0, 0) &&
- errno != EINVAL && errno != EPERM) {
- err("Couldn't drop cap %i from bounding set: %s",
- i, strerror(errno));
- exit(EXIT_FAILURE);
- }
+ errno != EINVAL && errno != EPERM)
+ errexit("Couldn't drop cap %i from bounding set: %s",
+ i, strerror(errno));
}
- if (syscall(SYS_capget, &hdr, data)) {
- err("Couldn't get current capabilities: %s", strerror(errno));
- exit(EXIT_FAILURE);
- }
+ if (syscall(SYS_capget, &hdr, data))
+ errexit("Couldn't get current capabilities: %s", strerror(errno));
for (i = 0; i < CAP_WORDS; i++)
data[i].inheritable = 0;
- if (syscall(SYS_capset, &hdr, data)) {
- err("Couldn't drop inheritable capabilities: %s",
- strerror(errno));
- exit(EXIT_FAILURE);
- }
+ if (syscall(SYS_capset, &hdr, data))
+ errexit("Couldn't drop inheritable capabilities: %s",
+ strerror(errno));
}
/**
@@ -229,46 +219,34 @@ void isolate_user(uid_t uid, gid_t gid, bool use_userns, const char *userns,
/* First set our UID & GID in the original namespace */
if (setgroups(0, NULL)) {
/* If we don't have CAP_SETGID, this will EPERM */
- if (errno != EPERM) {
- err("Can't drop supplementary groups: %s",
- strerror(errno));
- exit(EXIT_FAILURE);
- }
+ if (errno != EPERM)
+ errexit("Can't drop supplementary groups: %s",
+ strerror(errno));
}
- if (setgid(gid) != 0) {
- err("Can't set GID to %u: %s", gid, strerror(errno));
- exit(EXIT_FAILURE);
- }
+ if (setgid(gid) != 0)
+ errexit("Can't set GID to %u: %s", gid, strerror(errno));
- if (setuid(uid) != 0) {
- err("Can't set UID to %u: %s", uid, strerror(errno));
- exit(EXIT_FAILURE);
- }
+ if (setuid(uid) != 0)
+ errexit("Can't set UID to %u: %s", uid, strerror(errno));
if (*userns) { /* If given a userns, join it */
int ufd;
ufd = open(userns, O_RDONLY | O_CLOEXEC);
- if (ufd < 0) {
- err("Couldn't open user namespace %s: %s",
- userns, strerror(errno));
- exit(EXIT_FAILURE);
- }
-
- if (setns(ufd, CLONE_NEWUSER) != 0) {
- err("Couldn't enter user namespace %s: %s",
- userns, strerror(errno));
- exit(EXIT_FAILURE);
- }
+ if (ufd < 0)
+ errexit("Couldn't open user namespace %s: %s",
+ userns, strerror(errno));
+
+ if (setns(ufd, CLONE_NEWUSER) != 0)
+ errexit("Couldn't enter user namespace %s: %s",
+ userns, strerror(errno));
close(ufd);
} else if (use_userns) { /* Create and join a new userns */
- if (unshare(CLONE_NEWUSER) != 0) {
- err("Couldn't create user namespace: %s", strerror(errno));
- exit(EXIT_FAILURE);
- }
+ if (unshare(CLONE_NEWUSER) != 0)
+ errexit("Couldn't create user namespace: %s", strerror(errno));
}
/* Joining a new userns gives us full capabilities; drop the
diff --git a/log.c b/log.c
index 4956914..983c82f 100644
--- a/log.c
+++ b/log.c
@@ -204,10 +204,8 @@ void logfile_init(const char *name, const char *path, size_t size)
log_file = open(path, O_CREAT | O_TRUNC | O_APPEND | O_RDWR | O_CLOEXEC,
S_IRUSR | S_IWUSR);
- if (log_file == -1) {
- err("Couldn't open log file %s: %s", path, strerror(errno));
- exit(EXIT_FAILURE);
- }
+ if (log_file == -1)
+ errexit("Couldn't open log file %s: %s", path, strerror(errno));
log_size = size ? size : LOGFILE_SIZE_DEFAULT;
diff --git a/netlink.c b/netlink.c
index 0850cbe..a6c6e1e 100644
--- a/netlink.c
+++ b/netlink.c
@@ -90,8 +90,7 @@ void nl_sock_init(const struct ctx *c, bool ns)
return;
fail:
- err("Failed to get netlink socket");
- exit(EXIT_FAILURE);
+ errexit("Failed to get netlink socket");
}
/**
diff --git a/passt.c b/passt.c
index cf010e8..443d51a 100644
--- a/passt.c
+++ b/passt.c
@@ -202,10 +202,8 @@ int main(int argc, char **argv)
name = basename(argv0);
if (strstr(name, "pasta")) {
sa.sa_handler = pasta_child_handler;
- if (sigaction(SIGCHLD, &sa, NULL) || signal(SIGPIPE, SIG_IGN)) {
- err("Couldn't install signal handlers");
- exit(EXIT_FAILURE);
- }
+ if (sigaction(SIGCHLD, &sa, NULL) || signal(SIGPIPE, SIG_IGN))
+ errexit("Couldn't install signal handlers");
c.mode = MODE_PASTA;
log_name = "pasta";
@@ -291,10 +289,8 @@ int main(int argc, char **argv)
}
}
- if (isolate_prefork(&c)) {
- err("Failed to sandbox process, exiting\n");
- exit(EXIT_FAILURE);
- }
+ if (isolate_prefork(&c))
+ errexit("Failed to sandbox process, exiting\n");
if (!c.foreground) {
__daemon(pidfile_fd, devnull_fd);
diff --git a/pasta.c b/pasta.c
index 528f02a..b1463c9 100644
--- a/pasta.c
+++ b/pasta.c
@@ -123,19 +123,15 @@ void pasta_open_ns(struct ctx *c, const char *netns)
int nfd = -1;
nfd = open(netns, O_RDONLY | O_CLOEXEC);
- if (nfd < 0) {
- err("Couldn't open network namespace %s", netns);
- exit(EXIT_FAILURE);
- }
+ if (nfd < 0)
+ errexit("Couldn't open network namespace %s", netns);
c->pasta_netns_fd = nfd;
NS_CALL(ns_check, c);
- if (c->pasta_netns_fd < 0) {
- err("Couldn't switch to pasta namespaces");
- exit(EXIT_FAILURE);
- }
+ if (c->pasta_netns_fd < 0)
+ errexit("Couldn't switch to pasta namespaces");
if (!c->no_netns_quit) {
char buf[PATH_MAX] = { 0 };
@@ -217,11 +213,10 @@ void pasta_start_ns(struct ctx *c, uid_t uid, gid_t gid,
arg.exe = "/bin/sh";
if ((size_t)snprintf(sh_arg0, sizeof(sh_arg0),
- "-%s", arg.exe) >= sizeof(sh_arg0)) {
- err("$SHELL is too long (%u bytes)",
- strlen(arg.exe));
- exit(EXIT_FAILURE);
- }
+ "-%s", arg.exe) >= sizeof(sh_arg0))
+ errexit("$SHELL is too long (%u bytes)",
+ strlen(arg.exe));
+
sh_argv[0] = sh_arg0;
arg.argv = sh_argv;
}
diff --git a/tap.c b/tap.c
index 2e603ed..edb3184 100644
--- a/tap.c
+++ b/tap.c
@@ -886,10 +886,8 @@ static void tap_sock_unix_init(struct ctx *c)
};
int i;
- if (fd < 0) {
- err("UNIX socket: %s", strerror(errno));
- exit(EXIT_FAILURE);
- }
+ if (fd < 0)
+ errexit("UNIX socket: %s", strerror(errno));
/* In passt mode, we don't know the guest's MAC until it sends
* us packets. Use the broadcast address so our first packets
@@ -907,18 +905,14 @@ static void tap_sock_unix_init(struct ctx *c)
snprintf(path, UNIX_PATH_MAX - 1, UNIX_SOCK_PATH, i);
ex = socket(AF_UNIX, SOCK_STREAM | SOCK_NONBLOCK, 0);
- if (ex < 0) {
- err("UNIX domain socket check: %s", strerror(errno));
- exit(EXIT_FAILURE);
- }
+ if (ex < 0)
+ errexit("UNIX domain socket check: %s", strerror(errno));
ret = connect(ex, (const struct sockaddr *)&addr, sizeof(addr));
if (!ret || (errno != ENOENT && errno != ECONNREFUSED &&
errno != EACCES)) {
- if (*c->sock_path) {
- err("Socket path %s already in use", path);
- exit(EXIT_FAILURE);
- }
+ if (*c->sock_path)
+ errexit("Socket path %s already in use", path);
close(ex);
continue;
@@ -931,10 +925,8 @@ static void tap_sock_unix_init(struct ctx *c)
break;
}
- if (i == UNIX_SOCK_MAX) {
- err("UNIX socket bind: %s", strerror(errno));
- exit(EXIT_FAILURE);
- }
+ if (i == UNIX_SOCK_MAX)
+ errexit("UNIX socket bind: %s", strerror(errno));
info("UNIX domain socket bound at %s\n", addr.sun_path);
@@ -1037,10 +1029,8 @@ static void tap_sock_tun_init(struct ctx *c)
struct epoll_event ev = { 0 };
NS_CALL(tap_ns_tun, c);
- if (tun_ns_fd == -1) {
- err("Failed to open tun socket in namespace");
- exit(EXIT_FAILURE);
- }
+ if (tun_ns_fd == -1)
+ errexit("Failed to open tun socket in namespace");
pasta_ns_conf(c);
--
@@ -886,10 +886,8 @@ static void tap_sock_unix_init(struct ctx *c)
};
int i;
- if (fd < 0) {
- err("UNIX socket: %s", strerror(errno));
- exit(EXIT_FAILURE);
- }
+ if (fd < 0)
+ errexit("UNIX socket: %s", strerror(errno));
/* In passt mode, we don't know the guest's MAC until it sends
* us packets. Use the broadcast address so our first packets
@@ -907,18 +905,14 @@ static void tap_sock_unix_init(struct ctx *c)
snprintf(path, UNIX_PATH_MAX - 1, UNIX_SOCK_PATH, i);
ex = socket(AF_UNIX, SOCK_STREAM | SOCK_NONBLOCK, 0);
- if (ex < 0) {
- err("UNIX domain socket check: %s", strerror(errno));
- exit(EXIT_FAILURE);
- }
+ if (ex < 0)
+ errexit("UNIX domain socket check: %s", strerror(errno));
ret = connect(ex, (const struct sockaddr *)&addr, sizeof(addr));
if (!ret || (errno != ENOENT && errno != ECONNREFUSED &&
errno != EACCES)) {
- if (*c->sock_path) {
- err("Socket path %s already in use", path);
- exit(EXIT_FAILURE);
- }
+ if (*c->sock_path)
+ errexit("Socket path %s already in use", path);
close(ex);
continue;
@@ -931,10 +925,8 @@ static void tap_sock_unix_init(struct ctx *c)
break;
}
- if (i == UNIX_SOCK_MAX) {
- err("UNIX socket bind: %s", strerror(errno));
- exit(EXIT_FAILURE);
- }
+ if (i == UNIX_SOCK_MAX)
+ errexit("UNIX socket bind: %s", strerror(errno));
info("UNIX domain socket bound at %s\n", addr.sun_path);
@@ -1037,10 +1029,8 @@ static void tap_sock_tun_init(struct ctx *c)
struct epoll_event ev = { 0 };
NS_CALL(tap_ns_tun, c);
- if (tun_ns_fd == -1) {
- err("Failed to open tun socket in namespace");
- exit(EXIT_FAILURE);
- }
+ if (tun_ns_fd == -1)
+ errexit("Failed to open tun socket in namespace");
pasta_ns_conf(c);
--
2.39.1
^ permalink raw reply related [flat|nested] 21+ messages in thread* Re: [PATCH v2 9/9] convert all remaining err() followed by exit() to errexit()
2023-02-08 17:48 ` [PATCH v2 9/9] convert all remaining err() followed by exit() to errexit() Laine Stump
@ 2023-02-09 17:45 ` Stefano Brivio
0 siblings, 0 replies; 21+ messages in thread
From: Stefano Brivio @ 2023-02-09 17:45 UTC (permalink / raw)
To: Laine Stump; +Cc: passt-dev, laine
On Wed, 8 Feb 2023 12:48:38 -0500
Laine Stump <laine@redhat.com> wrote:
> This actually leaves us with 0 uses of err(), but someone could want
> to use it in the future, so may as well leave it around.
>
> Signed-off-by: Laine Stump <laine@redhat.com>
> ---
> isolation.c | 78 +++++++++++++++++++----------------------------------
> log.c | 6 ++---
> netlink.c | 3 +--
> passt.c | 12 +++------
> pasta.c | 21 ++++++---------
> tap.c | 30 +++++++--------------
> 6 files changed, 53 insertions(+), 97 deletions(-)
>
> diff --git a/isolation.c b/isolation.c
> index 4e6637d..0f709c6 100644
> --- a/isolation.c
> +++ b/isolation.c
> @@ -103,10 +103,8 @@ static void drop_caps_ep_except(uint64_t keep)
> struct __user_cap_data_struct data[CAP_WORDS];
> int i;
>
> - if (syscall(SYS_capget, &hdr, data)) {
> - err("Couldn't get current capabilities: %s", strerror(errno));
> - exit(EXIT_FAILURE);
> - }
> + if (syscall(SYS_capget, &hdr, data))
> + errexit("Couldn't get current capabilities: %s", strerror(errno));
Split before strerror().
>
> for (i = 0; i < CAP_WORDS; i++) {
> uint32_t mask = keep >> (32 * i);
> @@ -115,10 +113,8 @@ static void drop_caps_ep_except(uint64_t keep)
> data[i].permitted &= mask;
> }
>
> - if (syscall(SYS_capset, &hdr, data)) {
> - err("Couldn't drop capabilities: %s", strerror(errno));
> - exit(EXIT_FAILURE);
> - }
> + if (syscall(SYS_capset, &hdr, data))
> + errexit("Couldn't drop capabilities: %s", strerror(errno));
> }
>
> /**
> @@ -154,26 +150,20 @@ static void clamp_caps(void)
> * normal operation, so carry on without it.
> */
> if (prctl(PR_CAPBSET_DROP, i, 0, 0, 0) &&
> - errno != EINVAL && errno != EPERM) {
> - err("Couldn't drop cap %i from bounding set: %s",
> - i, strerror(errno));
> - exit(EXIT_FAILURE);
> - }
> + errno != EINVAL && errno != EPERM)
> + errexit("Couldn't drop cap %i from bounding set: %s",
> + i, strerror(errno));
> }
>
> - if (syscall(SYS_capget, &hdr, data)) {
> - err("Couldn't get current capabilities: %s", strerror(errno));
> - exit(EXIT_FAILURE);
> - }
> + if (syscall(SYS_capget, &hdr, data))
> + errexit("Couldn't get current capabilities: %s", strerror(errno));
Same here.
>
> for (i = 0; i < CAP_WORDS; i++)
> data[i].inheritable = 0;
>
> - if (syscall(SYS_capset, &hdr, data)) {
> - err("Couldn't drop inheritable capabilities: %s",
> - strerror(errno));
> - exit(EXIT_FAILURE);
> - }
> + if (syscall(SYS_capset, &hdr, data))
> + errexit("Couldn't drop inheritable capabilities: %s",
> + strerror(errno));
> }
>
> /**
> @@ -229,46 +219,34 @@ void isolate_user(uid_t uid, gid_t gid, bool use_userns, const char *userns,
> /* First set our UID & GID in the original namespace */
> if (setgroups(0, NULL)) {
> /* If we don't have CAP_SETGID, this will EPERM */
> - if (errno != EPERM) {
> - err("Can't drop supplementary groups: %s",
> - strerror(errno));
> - exit(EXIT_FAILURE);
> - }
> + if (errno != EPERM)
> + errexit("Can't drop supplementary groups: %s",
> + strerror(errno));
> }
>
> - if (setgid(gid) != 0) {
> - err("Can't set GID to %u: %s", gid, strerror(errno));
> - exit(EXIT_FAILURE);
> - }
> + if (setgid(gid) != 0)
> + errexit("Can't set GID to %u: %s", gid, strerror(errno));
>
> - if (setuid(uid) != 0) {
> - err("Can't set UID to %u: %s", uid, strerror(errno));
> - exit(EXIT_FAILURE);
> - }
> + if (setuid(uid) != 0)
> + errexit("Can't set UID to %u: %s", uid, strerror(errno));
>
> if (*userns) { /* If given a userns, join it */
> int ufd;
>
> ufd = open(userns, O_RDONLY | O_CLOEXEC);
> - if (ufd < 0) {
> - err("Couldn't open user namespace %s: %s",
> - userns, strerror(errno));
> - exit(EXIT_FAILURE);
> - }
> -
> - if (setns(ufd, CLONE_NEWUSER) != 0) {
> - err("Couldn't enter user namespace %s: %s",
> - userns, strerror(errno));
> - exit(EXIT_FAILURE);
> - }
> + if (ufd < 0)
> + errexit("Couldn't open user namespace %s: %s",
> + userns, strerror(errno));
> +
> + if (setns(ufd, CLONE_NEWUSER) != 0)
> + errexit("Couldn't enter user namespace %s: %s",
> + userns, strerror(errno));
>
> close(ufd);
>
> } else if (use_userns) { /* Create and join a new userns */
> - if (unshare(CLONE_NEWUSER) != 0) {
> - err("Couldn't create user namespace: %s", strerror(errno));
> - exit(EXIT_FAILURE);
> - }
> + if (unshare(CLONE_NEWUSER) != 0)
> + errexit("Couldn't create user namespace: %s", strerror(errno));
And here.
> }
>
> /* Joining a new userns gives us full capabilities; drop the
> diff --git a/log.c b/log.c
> index 4956914..983c82f 100644
> --- a/log.c
> +++ b/log.c
> @@ -204,10 +204,8 @@ void logfile_init(const char *name, const char *path, size_t size)
>
> log_file = open(path, O_CREAT | O_TRUNC | O_APPEND | O_RDWR | O_CLOEXEC,
> S_IRUSR | S_IWUSR);
> - if (log_file == -1) {
> - err("Couldn't open log file %s: %s", path, strerror(errno));
> - exit(EXIT_FAILURE);
> - }
> + if (log_file == -1)
> + errexit("Couldn't open log file %s: %s", path, strerror(errno));
>
> log_size = size ? size : LOGFILE_SIZE_DEFAULT;
>
> diff --git a/netlink.c b/netlink.c
> index 0850cbe..a6c6e1e 100644
> --- a/netlink.c
> +++ b/netlink.c
> @@ -90,8 +90,7 @@ void nl_sock_init(const struct ctx *c, bool ns)
> return;
>
> fail:
> - err("Failed to get netlink socket");
> - exit(EXIT_FAILURE);
> + errexit("Failed to get netlink socket");
> }
>
> /**
> diff --git a/passt.c b/passt.c
> index cf010e8..443d51a 100644
> --- a/passt.c
> +++ b/passt.c
> @@ -202,10 +202,8 @@ int main(int argc, char **argv)
> name = basename(argv0);
> if (strstr(name, "pasta")) {
> sa.sa_handler = pasta_child_handler;
> - if (sigaction(SIGCHLD, &sa, NULL) || signal(SIGPIPE, SIG_IGN)) {
> - err("Couldn't install signal handlers");
> - exit(EXIT_FAILURE);
> - }
> + if (sigaction(SIGCHLD, &sa, NULL) || signal(SIGPIPE, SIG_IGN))
> + errexit("Couldn't install signal handlers");
>
> c.mode = MODE_PASTA;
> log_name = "pasta";
> @@ -291,10 +289,8 @@ int main(int argc, char **argv)
> }
> }
>
> - if (isolate_prefork(&c)) {
> - err("Failed to sandbox process, exiting\n");
> - exit(EXIT_FAILURE);
> - }
> + if (isolate_prefork(&c))
> + errexit("Failed to sandbox process, exiting\n");
>
> if (!c.foreground) {
> __daemon(pidfile_fd, devnull_fd);
> diff --git a/pasta.c b/pasta.c
> index 528f02a..b1463c9 100644
> --- a/pasta.c
> +++ b/pasta.c
> @@ -123,19 +123,15 @@ void pasta_open_ns(struct ctx *c, const char *netns)
> int nfd = -1;
>
> nfd = open(netns, O_RDONLY | O_CLOEXEC);
> - if (nfd < 0) {
> - err("Couldn't open network namespace %s", netns);
> - exit(EXIT_FAILURE);
> - }
> + if (nfd < 0)
> + errexit("Couldn't open network namespace %s", netns);
>
> c->pasta_netns_fd = nfd;
>
> NS_CALL(ns_check, c);
>
> - if (c->pasta_netns_fd < 0) {
> - err("Couldn't switch to pasta namespaces");
> - exit(EXIT_FAILURE);
> - }
> + if (c->pasta_netns_fd < 0)
> + errexit("Couldn't switch to pasta namespaces");
>
> if (!c->no_netns_quit) {
> char buf[PATH_MAX] = { 0 };
> @@ -217,11 +213,10 @@ void pasta_start_ns(struct ctx *c, uid_t uid, gid_t gid,
> arg.exe = "/bin/sh";
>
> if ((size_t)snprintf(sh_arg0, sizeof(sh_arg0),
> - "-%s", arg.exe) >= sizeof(sh_arg0)) {
> - err("$SHELL is too long (%u bytes)",
> - strlen(arg.exe));
> - exit(EXIT_FAILURE);
> - }
> + "-%s", arg.exe) >= sizeof(sh_arg0))
> + errexit("$SHELL is too long (%u bytes)",
> + strlen(arg.exe));
> +
> sh_argv[0] = sh_arg0;
> arg.argv = sh_argv;
> }
> diff --git a/tap.c b/tap.c
> index 2e603ed..edb3184 100644
> --- a/tap.c
> +++ b/tap.c
> @@ -886,10 +886,8 @@ static void tap_sock_unix_init(struct ctx *c)
> };
> int i;
>
> - if (fd < 0) {
> - err("UNIX socket: %s", strerror(errno));
> - exit(EXIT_FAILURE);
> - }
> + if (fd < 0)
> + errexit("UNIX socket: %s", strerror(errno));
>
> /* In passt mode, we don't know the guest's MAC until it sends
> * us packets. Use the broadcast address so our first packets
> @@ -907,18 +905,14 @@ static void tap_sock_unix_init(struct ctx *c)
> snprintf(path, UNIX_PATH_MAX - 1, UNIX_SOCK_PATH, i);
>
> ex = socket(AF_UNIX, SOCK_STREAM | SOCK_NONBLOCK, 0);
> - if (ex < 0) {
> - err("UNIX domain socket check: %s", strerror(errno));
> - exit(EXIT_FAILURE);
> - }
> + if (ex < 0)
> + errexit("UNIX domain socket check: %s", strerror(errno));
And here.
>
> ret = connect(ex, (const struct sockaddr *)&addr, sizeof(addr));
> if (!ret || (errno != ENOENT && errno != ECONNREFUSED &&
> errno != EACCES)) {
> - if (*c->sock_path) {
> - err("Socket path %s already in use", path);
> - exit(EXIT_FAILURE);
> - }
> + if (*c->sock_path)
> + errexit("Socket path %s already in use", path);
>
> close(ex);
> continue;
> @@ -931,10 +925,8 @@ static void tap_sock_unix_init(struct ctx *c)
> break;
> }
>
> - if (i == UNIX_SOCK_MAX) {
> - err("UNIX socket bind: %s", strerror(errno));
> - exit(EXIT_FAILURE);
> - }
> + if (i == UNIX_SOCK_MAX)
> + errexit("UNIX socket bind: %s", strerror(errno));
>
> info("UNIX domain socket bound at %s\n", addr.sun_path);
>
> @@ -1037,10 +1029,8 @@ static void tap_sock_tun_init(struct ctx *c)
> struct epoll_event ev = { 0 };
>
> NS_CALL(tap_ns_tun, c);
> - if (tun_ns_fd == -1) {
> - err("Failed to open tun socket in namespace");
> - exit(EXIT_FAILURE);
> - }
> + if (tun_ns_fd == -1)
> + errexit("Failed to open tun socket in namespace");
>
> pasta_ns_conf(c);
>
--
Stefano
^ permalink raw reply [flat|nested] 21+ messages in thread