From: David Gibson <david@gibson.dropbear.id.au>
To: passt-dev@passt.top
Subject: Re: [PATCH 1/8] Move logging functions to a new file, log.c
Date: Fri, 07 Oct 2022 17:13:45 +1100 [thread overview]
Message-ID: <Yz/DmSaaX+XmEuen@yekko> (raw)
In-Reply-To: <20221007004742.1188933-2-sbrivio@redhat.com>
[-- Attachment #1: Type: text/plain, Size: 16273 bytes --]
On Fri, Oct 07, 2022 at 02:47:35AM +0200, Stefano Brivio wrote:
> Logging to file is going to add some further complexity that we don't
> want to squeeze into util.c.
>
> Signed-off-by: Stefano Brivio <sbrivio(a)redhat.com>
Reviewed-by: David Gibson <david(a)gibson.dropbear.id.au>
> ---
> Makefile | 4 +-
> conf.c | 1 +
> dhcp.c | 1 +
> dhcpv6.c | 1 +
> icmp.c | 1 +
> isolation.c | 1 +
> log.c | 143 +++++++++++++++++++++++++++++++++++++++++++++++++++
> log.h | 26 ++++++++++
> ndp.c | 1 +
> netlink.c | 1 +
> packet.c | 1 +
> passt.c | 1 +
> pasta.c | 1 +
> pcap.c | 1 +
> tap.c | 1 +
> tcp.c | 1 +
> tcp_splice.c | 1 +
> udp.c | 1 +
> util.c | 121 +------------------------------------------
> util.h | 16 ------
> 20 files changed, 187 insertions(+), 138 deletions(-)
> create mode 100644 log.c
> create mode 100644 log.h
>
> diff --git a/Makefile b/Makefile
> index d4b623f..74bbfeb 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -33,8 +33,8 @@ FLAGS += -DRLIMIT_STACK_VAL=$(RLIMIT_STACK_VAL)
> FLAGS += -DARCH=\"$(TARGET_ARCH)\"
>
> PASST_SRCS = arch.c arp.c checksum.c conf.c dhcp.c dhcpv6.c icmp.c igmp.c \
> - isolation.c lineread.c mld.c ndp.c netlink.c packet.c passt.c pasta.c \
> - pcap.c siphash.c tap.c tcp.c tcp_splice.c udp.c util.c
> + isolation.c lineread.c log.c mld.c ndp.c netlink.c packet.c passt.c \
> + pasta.c pcap.c siphash.c tap.c tcp.c tcp_splice.c udp.c util.c
> QRAP_SRCS = qrap.c
> SRCS = $(PASST_SRCS) $(QRAP_SRCS)
>
> diff --git a/conf.c b/conf.c
> index 1537dbf..ba1d8de 100644
> --- a/conf.c
> +++ b/conf.c
> @@ -40,6 +40,7 @@
> #include "pasta.h"
> #include "lineread.h"
> #include "isolation.h"
> +#include "log.h"
>
> /**
> * get_bound_ports() - Get maps of ports with bound sockets
> diff --git a/dhcp.c b/dhcp.c
> index 7ad1319..7f0cc0b 100644
> --- a/dhcp.c
> +++ b/dhcp.c
> @@ -29,6 +29,7 @@
> #include "packet.h"
> #include "passt.h"
> #include "tap.h"
> +#include "log.h"
> #include "dhcp.h"
>
> /**
> diff --git a/dhcpv6.c b/dhcpv6.c
> index 8a1d4a6..e7640ce 100644
> --- a/dhcpv6.c
> +++ b/dhcpv6.c
> @@ -30,6 +30,7 @@
> #include "util.h"
> #include "passt.h"
> #include "tap.h"
> +#include "log.h"
>
> /**
> * struct opt_hdr - DHCPv6 option header
> diff --git a/icmp.c b/icmp.c
> index 29170aa..f02f89f 100644
> --- a/icmp.c
> +++ b/icmp.c
> @@ -35,6 +35,7 @@
> #include "util.h"
> #include "passt.h"
> #include "tap.h"
> +#include "log.h"
> #include "icmp.h"
>
> #define ICMP_ECHO_TIMEOUT 60 /* s, timeout for ICMP socket activity */
> diff --git a/isolation.c b/isolation.c
> index 124dea4..b54c325 100644
> --- a/isolation.c
> +++ b/isolation.c
> @@ -42,6 +42,7 @@
> #include "util.h"
> #include "seccomp.h"
> #include "passt.h"
> +#include "log.h"
> #include "isolation.h"
>
> /**
> diff --git a/log.c b/log.c
> new file mode 100644
> index 0000000..54a7cbb
> --- /dev/null
> +++ b/log.c
> @@ -0,0 +1,143 @@
> +// SPDX-License-Identifier: AGPL-3.0-or-later
> +
> +/* PASST - Plug A Simple Socket Transport
> + * for qemu/UNIX domain socket mode
> + *
> + * PASTA - Pack A Subtle Tap Abstraction
> + * for network namespace/tap device mode
> + *
> + * log.c - Logging functions
> + *
> + * Copyright (c) 2020-2022 Red Hat GmbH
> + * Author: Stefano Brivio <sbrivio(a)redhat.com>
> + */
> +
> +#include <stdio.h>
> +#include <stdlib.h>
> +#include <unistd.h>
> +#include <string.h>
> +#include <time.h>
> +#include <syslog.h>
> +#include <stdarg.h>
> +#include <sys/socket.h>
> +#include <sys/un.h>
> +
> +#include "log.h"
> +
> +/* For __openlog() and __setlogmask() wrappers, and passt_vsyslog() */
> +static int log_mask;
> +static int log_sock = -1;
> +static char log_ident[BUFSIZ];
> +static int log_opt;
> +static time_t log_debug_start;
> +int log_trace;
> +
> +#define logfn(name, level) \
> +void name(const char *format, ...) { \
> + struct timespec tp; \
> + va_list args; \
> + \
> + if (setlogmask(0) & LOG_MASK(LOG_DEBUG)) { \
> + clock_gettime(CLOCK_REALTIME, &tp); \
> + fprintf(stderr, "%li.%04li: ", \
> + tp.tv_sec - log_debug_start, \
> + tp.tv_nsec / (100L * 1000)); \
> + } else { \
> + va_start(args, format); \
> + passt_vsyslog(level, format, args); \
> + va_end(args); \
> + } \
> + \
> + if (setlogmask(0) & LOG_MASK(LOG_DEBUG) || \
> + setlogmask(0) == LOG_MASK(LOG_EMERG)) { \
> + va_start(args, format); \
> + (void)vfprintf(stderr, format, args); \
> + va_end(args); \
> + if (format[strlen(format)] != '\n') \
> + fprintf(stderr, "\n"); \
> + } \
> +}
> +
> +logfn(err, LOG_ERR)
> +logfn(warn, LOG_WARNING)
> +logfn(info, LOG_INFO)
> +logfn(debug, LOG_DEBUG)
> +
> +void trace_init(int enable)
> +{
> + log_trace = enable;
> +}
> +
> +/**
> + * __openlog() - Non-optional openlog() wrapper, to allow custom vsyslog()
> + * @ident: openlog() identity (program name)
> + * @option: openlog() options
> + * @facility: openlog() facility (LOG_DAEMON)
> + */
> +void __openlog(const char *ident, int option, int facility)
> +{
> + struct timespec tp;
> +
> + clock_gettime(CLOCK_REALTIME, &tp);
> + log_debug_start = tp.tv_sec;
> +
> + if (log_sock < 0) {
> + struct sockaddr_un a = { .sun_family = AF_UNIX, };
> +
> + log_sock = socket(AF_UNIX, SOCK_DGRAM | SOCK_CLOEXEC, 0);
> + if (log_sock < 0)
> + return;
> +
> + strncpy(a.sun_path, _PATH_LOG, sizeof(a.sun_path));
> + if (connect(log_sock, (const struct sockaddr *)&a, sizeof(a))) {
> + close(log_sock);
> + log_sock = -1;
> + return;
> + }
> + }
> +
> + log_mask |= facility;
> + strncpy(log_ident, ident, sizeof(log_ident) - 1);
> + log_opt = option;
> +
> + openlog(ident, option, facility);
> +}
> +
> +/**
> + * __setlogmask() - setlogmask() wrapper, to allow custom vsyslog()
> + * @mask: Same as setlogmask() mask
> + */
> +void __setlogmask(int mask)
> +{
> + log_mask = mask;
> + setlogmask(mask);
> +}
> +
> +/**
> + * passt_vsyslog() - vsyslog() implementation not using heap memory
> + * @pri: Facility and level map, same as priority for vsyslog()
> + * @format: Same as vsyslog() format
> + * @ap: Same as vsyslog() ap
> + */
> +void passt_vsyslog(int pri, const char *format, va_list ap)
> +{
> + char buf[BUFSIZ];
> + int n;
> +
> + if (!(LOG_MASK(LOG_PRI(pri)) & log_mask))
> + return;
> +
> + /* Send without name and timestamp, the system logger should add them */
> + n = snprintf(buf, BUFSIZ, "<%i> ", pri);
> +
> + n += vsnprintf(buf + n, BUFSIZ - n, format, ap);
> +
> + if (format[strlen(format)] != '\n')
> + n += snprintf(buf + n, BUFSIZ - n, "\n");
> +
> + if (log_opt & LOG_PERROR)
> + fprintf(stderr, "%s", buf + sizeof("<0>"));
> +
> + if (send(log_sock, buf, n, 0) != n)
> + fprintf(stderr, "Failed to send %i bytes to syslog\n", n);
> +}
> diff --git a/log.h b/log.h
> new file mode 100644
> index 0000000..70cad6f
> --- /dev/null
> +++ b/log.h
> @@ -0,0 +1,26 @@
> +/* SPDX-License-Identifier: AGPL-3.0-or-later
> + * Copyright (c) 2022 Red Hat GmbH
> + * Author: Stefano Brivio <sbrivio(a)redhat.com>
> + */
> +
> +#ifndef LOG_H
> +#define LOG_H
> +
> +void err(const char *format, ...);
> +void warn(const char *format, ...);
> +void info(const char *format, ...);
> +void debug(const char *format, ...);
> +
> +extern int log_trace;
> +void trace_init(int enable);
> +#define trace(format, ...) \
> + do { \
> + if (log_trace) \
> + debug(format, ##__VA_ARGS__); \
> + } while (0)
> +
> +void __openlog(const char *ident, int option, int facility);
> +void passt_vsyslog(int pri, const char *format, va_list ap);
> +void __setlogmask(int mask);
> +
> +#endif /* LOG_H */
> diff --git a/ndp.c b/ndp.c
> index 29c4b14..dec36a9 100644
> --- a/ndp.c
> +++ b/ndp.c
> @@ -30,6 +30,7 @@
> #include "util.h"
> #include "passt.h"
> #include "tap.h"
> +#include "log.h"
>
> #define RS 133
> #define RA 134
> diff --git a/netlink.c b/netlink.c
> index 15ce213..9719e91 100644
> --- a/netlink.c
> +++ b/netlink.c
> @@ -30,6 +30,7 @@
>
> #include "util.h"
> #include "passt.h"
> +#include "log.h"
> #include "netlink.h"
>
> /* Socket in init, in target namespace, sequence (just needs to be monotonic) */
> diff --git a/packet.c b/packet.c
> index 6d10ec1..3f82e84 100644
> --- a/packet.c
> +++ b/packet.c
> @@ -20,6 +20,7 @@
>
> #include "packet.h"
> #include "util.h"
> +#include "log.h"
>
> /**
> * packet_add_do() - Add data as packet descriptor to given pool
> diff --git a/passt.c b/passt.c
> index 2c4a986..0c561b2 100644
> --- a/passt.c
> +++ b/passt.c
> @@ -44,6 +44,7 @@
> #include "conf.h"
> #include "pasta.h"
> #include "arch.h"
> +#include "log.h"
>
> #define EPOLL_EVENTS 8
>
> diff --git a/pasta.c b/pasta.c
> index 1dd8267..1887dd2 100644
> --- a/pasta.c
> +++ b/pasta.c
> @@ -44,6 +44,7 @@
> #include "passt.h"
> #include "isolation.h"
> #include "netlink.h"
> +#include "log.h"
>
> /* PID of child, in case we created a namespace */
> static int pasta_child_pid;
> diff --git a/pcap.c b/pcap.c
> index 7a48baf..836688d 100644
> --- a/pcap.c
> +++ b/pcap.c
> @@ -30,6 +30,7 @@
>
> #include "util.h"
> #include "passt.h"
> +#include "log.h"
>
> #define PCAP_VERSION_MINOR 4
>
> diff --git a/tap.c b/tap.c
> index 5540c18..bdcc161 100644
> --- a/tap.c
> +++ b/tap.c
> @@ -52,6 +52,7 @@
> #include "netlink.h"
> #include "pasta.h"
> #include "packet.h"
> +#include "log.h"
>
> /* IPv4 (plus ARP) and IPv6 message batches from tap/guest to IP handlers */
> static PACKET_POOL_NOINIT(pool_tap4, TAP_MSGS, pkt_buf);
> diff --git a/tcp.c b/tcp.c
> index 8ec991c..830dc88 100644
> --- a/tcp.c
> +++ b/tcp.c
> @@ -298,6 +298,7 @@
> #include "pcap.h"
> #include "conf.h"
> #include "tcp_splice.h"
> +#include "log.h"
>
> #define TCP_FRAMES_MEM 128
> #define TCP_FRAMES \
> diff --git a/tcp_splice.c b/tcp_splice.c
> index edbcfd4..4a015d0 100644
> --- a/tcp_splice.c
> +++ b/tcp_splice.c
> @@ -47,6 +47,7 @@
>
> #include "util.h"
> #include "passt.h"
> +#include "log.h"
>
> #define MAX_PIPE_SIZE (8UL * 1024 * 1024)
> #define TCP_SPLICE_MAX_CONNS (128 * 1024)
> diff --git a/udp.c b/udp.c
> index d1be622..5422fdd 100644
> --- a/udp.c
> +++ b/udp.c
> @@ -116,6 +116,7 @@
> #include "passt.h"
> #include "tap.h"
> #include "pcap.h"
> +#include "log.h"
>
> #define UDP_CONN_TIMEOUT 180 /* s, timeout for ephemeral or local bind */
> #define UDP_SPLICE_FRAMES 32
> diff --git a/util.c b/util.c
> index 6b86ead..b366167 100644
> --- a/util.c
> +++ b/util.c
> @@ -19,8 +19,6 @@
> #include <net/ethernet.h>
> #include <sys/epoll.h>
> #include <fcntl.h>
> -#include <syslog.h>
> -#include <stdarg.h>
> #include <string.h>
> #include <time.h>
> #include <errno.h>
> @@ -29,124 +27,7 @@
> #include "passt.h"
> #include "packet.h"
> #include "lineread.h"
> -
> -/* For __openlog() and __setlogmask() wrappers, and passt_vsyslog() */
> -static int log_mask;
> -static int log_sock = -1;
> -static char log_ident[BUFSIZ];
> -static int log_opt;
> -static time_t log_debug_start;
> -int log_trace;
> -
> -#define logfn(name, level) \
> -void name(const char *format, ...) { \
> - struct timespec tp; \
> - va_list args; \
> - \
> - if (setlogmask(0) & LOG_MASK(LOG_DEBUG)) { \
> - clock_gettime(CLOCK_REALTIME, &tp); \
> - fprintf(stderr, "%li.%04li: ", \
> - tp.tv_sec - log_debug_start, \
> - tp.tv_nsec / (100L * 1000)); \
> - } else { \
> - va_start(args, format); \
> - passt_vsyslog(level, format, args); \
> - va_end(args); \
> - } \
> - \
> - if (setlogmask(0) & LOG_MASK(LOG_DEBUG) || \
> - setlogmask(0) == LOG_MASK(LOG_EMERG)) { \
> - va_start(args, format); \
> - (void)vfprintf(stderr, format, args); \
> - va_end(args); \
> - if (format[strlen(format)] != '\n') \
> - fprintf(stderr, "\n"); \
> - } \
> -}
> -
> -logfn(err, LOG_ERR)
> -logfn(warn, LOG_WARNING)
> -logfn(info, LOG_INFO)
> -logfn(debug, LOG_DEBUG)
> -
> -void trace_init(int enable)
> -{
> - log_trace = enable;
> -}
> -
> -/**
> - * __openlog() - Non-optional openlog() wrapper, to allow custom vsyslog()
> - * @ident: openlog() identity (program name)
> - * @option: openlog() options
> - * @facility: openlog() facility (LOG_DAEMON)
> - */
> -void __openlog(const char *ident, int option, int facility)
> -{
> - struct timespec tp;
> -
> - clock_gettime(CLOCK_REALTIME, &tp);
> - log_debug_start = tp.tv_sec;
> -
> - if (log_sock < 0) {
> - struct sockaddr_un a = { .sun_family = AF_UNIX, };
> -
> - log_sock = socket(AF_UNIX, SOCK_DGRAM | SOCK_CLOEXEC, 0);
> - if (log_sock < 0)
> - return;
> -
> - strncpy(a.sun_path, _PATH_LOG, sizeof(a.sun_path));
> - if (connect(log_sock, (const struct sockaddr *)&a, sizeof(a))) {
> - close(log_sock);
> - log_sock = -1;
> - return;
> - }
> - }
> -
> - log_mask |= facility;
> - strncpy(log_ident, ident, sizeof(log_ident) - 1);
> - log_opt = option;
> -
> - openlog(ident, option, facility);
> -}
> -
> -/**
> - * __setlogmask() - setlogmask() wrapper, to allow custom vsyslog()
> - * @mask: Same as setlogmask() mask
> - */
> -void __setlogmask(int mask)
> -{
> - log_mask = mask;
> - setlogmask(mask);
> -}
> -
> -/**
> - * passt_vsyslog() - vsyslog() implementation not using heap memory
> - * @pri: Facility and level map, same as priority for vsyslog()
> - * @format: Same as vsyslog() format
> - * @ap: Same as vsyslog() ap
> - */
> -void passt_vsyslog(int pri, const char *format, va_list ap)
> -{
> - char buf[BUFSIZ];
> - int n;
> -
> - if (!(LOG_MASK(LOG_PRI(pri)) & log_mask))
> - return;
> -
> - /* Send without name and timestamp, the system logger should add them */
> - n = snprintf(buf, BUFSIZ, "<%i> ", pri);
> -
> - n += vsnprintf(buf + n, BUFSIZ - n, format, ap);
> -
> - if (format[strlen(format)] != '\n')
> - n += snprintf(buf + n, BUFSIZ - n, "\n");
> -
> - if (log_opt & LOG_PERROR)
> - fprintf(stderr, "%s", buf + sizeof("<0>"));
> -
> - if (send(log_sock, buf, n, 0) != n)
> - fprintf(stderr, "Failed to send %i bytes to syslog\n", n);
> -}
> +#include "log.h"
>
> #define IPV6_NH_OPT(nh) \
> ((nh) == 0 || (nh) == 43 || (nh) == 44 || (nh) == 50 || \
> diff --git a/util.h b/util.h
> index 0c06e34..1adbf04 100644
> --- a/util.h
> +++ b/util.h
> @@ -6,19 +6,6 @@
> #ifndef UTIL_H
> #define UTIL_H
>
> -void err(const char *format, ...);
> -void warn(const char *format, ...);
> -void info(const char *format, ...);
> -void debug(const char *format, ...);
> -
> -extern int log_trace;
> -void trace_init(int enable);
> -#define trace(format, ...) \
> - do { \
> - if (log_trace) \
> - debug(format, ##__VA_ARGS__); \
> - } while (0)
> -
> #ifndef SECCOMP_RET_KILL_PROCESS
> #define SECCOMP_RET_KILL_PROCESS SECCOMP_RET_KILL
> #endif
> @@ -196,9 +183,6 @@ struct ipv6_opt_hdr {
>
> /* cppcheck-suppress funcArgNamesDifferent */
> __attribute__ ((weak)) int ffsl(long int i) { return __builtin_ffsl(i); }
> -void __openlog(const char *ident, int option, int facility);
> -void passt_vsyslog(int pri, const char *format, va_list ap);
> -void __setlogmask(int mask);
> char *ipv6_l4hdr(const struct pool *p, int index, size_t offset, uint8_t *proto,
> size_t *dlen);
> int sock_l4(const struct ctx *c, int af, uint8_t proto,
--
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 --]
next prev parent reply other threads:[~2022-10-07 6:13 UTC|newest]
Thread overview: 29+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-10-07 0:47 [PATCH 0/8] Add support for log file, version display, and tests Stefano Brivio
2022-10-07 0:47 ` [PATCH 1/8] Move logging functions to a new file, log.c Stefano Brivio
2022-10-07 6:13 ` David Gibson [this message]
2022-10-07 0:47 ` [PATCH 2/8] conf: Drop duplicate, diverging optstring assignments Stefano Brivio
2022-10-07 6:20 ` David Gibson
2022-10-07 0:47 ` [PATCH 3/8] passt.h: Include netinet/if_ether.h before struct ctx declaration Stefano Brivio
2022-10-07 6:23 ` David Gibson
2022-10-07 7:44 ` Stefano Brivio
2022-10-07 8:40 ` David Gibson
2022-10-07 11:36 ` Stefano Brivio
2022-10-07 0:47 ` [PATCH 4/8] log, conf: Add support for logging to file Stefano Brivio
2022-10-07 6:51 ` David Gibson
2022-10-07 8:11 ` Stefano Brivio
2022-10-07 8:57 ` David Gibson
2022-10-07 10:27 ` Stefano Brivio
2022-10-07 0:47 ` [PATCH 5/8] log: Add missing function comment for trace_init() Stefano Brivio
2022-10-07 6:52 ` David Gibson
2022-10-07 0:47 ` [PATCH 6/8] conf, log, Makefile: Add versioning information Stefano Brivio
2022-10-07 6:57 ` David Gibson
2022-10-07 8:15 ` Stefano Brivio
2022-10-07 9:03 ` David Gibson
2022-10-07 10:12 ` Stefano Brivio
2022-10-07 0:47 ` [PATCH 7/8] util: Check return value of lseek() while reading bound ports from procfs Stefano Brivio
2022-10-07 6:58 ` David Gibson
2022-10-07 0:47 ` [PATCH 8/8] test: Add log file tests for pasta plus corresponding layout and setup Stefano Brivio
2022-10-07 8:37 ` David Gibson
2022-10-07 9:47 ` Stefano Brivio
2022-10-24 21:00 ` Stefano Brivio
2022-10-26 7:26 ` Stefano Brivio
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=Yz/DmSaaX+XmEuen@yekko \
--to=david@gibson.dropbear.id.au \
--cc=passt-dev@passt.top \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
Code repositories for project(s) associated with this public inbox
https://passt.top/passt
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for IMAP folder(s).