On Mon, Oct 28, 2024 at 11:00:41AM +0100, Stefano Brivio wrote: > In pcap_init(), we should always open the packet capture file with > O_CLOEXEC, even if we're not running in foreground: O_CLOEXEC means > close-on-exec, not close-on-fork. > > In logfile_init() and pidfile_open(), the fact that we pass a third > 'mode' argument to open() seems to confuse the android-cloexec-open > checker in LLVM versions from 16 to 19 (at least). > > The checker is suggesting to add O_CLOEXEC to 'mode', and not in > 'flags', where we already have it. .. well.. the checker with the googletest package installed, anyway :/ > Add a suppression for clang-tidy and a comment, and avoid repeating > those three time by adding a new helper, output_file_open(). > > Signed-off-by: Stefano Brivio Reviewed-by: David Gibson > --- > conf.c | 3 ++- > log.c | 3 +-- > pcap.c | 7 ++----- > util.c | 26 ++++++++++---------------- > util.h | 2 +- > 5 files changed, 16 insertions(+), 25 deletions(-) > > diff --git a/conf.c b/conf.c > index 4db7c64..b28f411 100644 > --- a/conf.c > +++ b/conf.c > @@ -1194,7 +1194,8 @@ static void conf_open_files(struct ctx *c) > if (c->mode != MODE_PASTA && c->fd_tap == -1) > c->fd_tap_listen = tap_sock_unix_open(c->sock_path); > > - c->pidfile_fd = pidfile_open(c->pidfile); > + if (*c->pidfile && (c->pidfile_fd = output_file_open(c->pidfile) < 0)) > + die_perror("Couldn't open PID file %s", c->pidfile); > } > > /** > diff --git a/log.c b/log.c > index dd25862..48db4d9 100644 > --- a/log.c > +++ b/log.c > @@ -410,8 +410,7 @@ void logfile_init(const char *name, const char *path, size_t size) > if (readlink("/proc/self/exe", exe, PATH_MAX - 1) < 0) > die_perror("Failed to read own /proc/self/exe link"); > > - log_file = open(path, O_CREAT | O_TRUNC | O_RDWR | O_CLOEXEC, > - S_IRUSR | S_IWUSR); > + log_file = output_file_open(path); > if (log_file == -1) > die_perror("Couldn't open log file %s", path); > > diff --git a/pcap.c b/pcap.c > index 6ee6cdf..a07eb33 100644 > --- a/pcap.c > +++ b/pcap.c > @@ -158,18 +158,15 @@ void pcap_iov(const struct iovec *iov, size_t iovcnt, size_t offset) > */ > void pcap_init(struct ctx *c) > { > - int flags = O_WRONLY | O_CREAT | O_TRUNC; > - > if (pcap_fd != -1) > return; > > if (!*c->pcap) > return; > > - flags |= c->foreground ? O_CLOEXEC : 0; > - pcap_fd = open(c->pcap, flags, S_IRUSR | S_IWUSR); > + pcap_fd = output_file_open(c->pcap); > if (pcap_fd == -1) { > - perror("open"); > + err_perror("Couldn't open pcap file %s", c->pcap); > return; > } > > diff --git a/util.c b/util.c > index 9cb705e..d838b34 100644 > --- a/util.c > +++ b/util.c > @@ -407,25 +407,19 @@ void pidfile_write(int fd, pid_t pid) > } > > /** > - * pidfile_open() - Open PID file if needed > - * @path: Path for PID file, empty string if no PID file is requested > + * output_file_open() - Open file for output, if needed > + * @path: Path for output file > * > - * Return: descriptor for PID file, -1 if path is NULL, won't return on failure > + * Return: file descriptor on success, -1 on failure with errno set by open() > */ > -int pidfile_open(const char *path) > +int output_file_open(const char *path) > { > - int fd; > - > - if (!*path) > - return -1; > - > - if ((fd = open(path, O_CREAT | O_TRUNC | O_WRONLY | O_CLOEXEC, > - S_IRUSR | S_IWUSR)) < 0) { > - perror("PID file open"); > - exit(EXIT_FAILURE); > - } > - > - return fd; > + /* We use O_CLOEXEC here, but clang-tidy as of LLVM 16 to 19 looks for > + * it in the 'mode' argument if we have one > + */ > + return open(path, O_CREAT | O_TRUNC | O_WRONLY | O_CLOEXEC, > + /* NOLINTNEXTLINE(android-cloexec-open) */ > + S_IRUSR | S_IWUSR); > } > > /** > diff --git a/util.h b/util.h > index 4f8b768..73b4a49 100644 > --- a/util.h > +++ b/util.h > @@ -193,7 +193,7 @@ char *line_read(char *buf, size_t len, int fd); > void ns_enter(const struct ctx *c); > bool ns_is_init(void); > int open_in_ns(const struct ctx *c, const char *path, int flags); > -int pidfile_open(const char *path); > +int output_file_open(const char *path); > void pidfile_write(int fd, pid_t pid); > int __daemon(int pidfile_fd, int devnull_fd); > int fls(unsigned long x); -- David Gibson (he or they) | 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