On Wed, May 22, 2024 at 10:59:04PM +0200, Stefano Brivio wrote:

libguestfs tools have a good reason to run as root: if the guest image is owned by root, it would be counterproductive to encourage users to invoke them as non-root, as it would require changing permissions or ownership of the image file.

And if they run as root, we'll start as root, too. Warn users we'll switch to 'nobody', but don't tell them what to do.

Reported-by: Richard W.M. Jones Signed-off-by: Stefano Brivio

Reviewed-by: David Gibson

--- conf.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/conf.c b/conf.c index 21d46fe..2e0d909 100644 --- a/conf.c +++ b/conf.c @@ -1093,7 +1093,7 @@ static void conf_ugid(char *runas, uid_t *uid, gid_t *gid) return;

/* ...otherwise use nobody:nobody */ - warn("Don't run as root. Changing to nobody..."); + warn("Started as root. Changing to nobody..."); { #ifndef GLIBC_NO_STATIC_NSS const struct passwd *pw;

-- 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

On Wed, May 22, 2024 at 10:59:05PM +0200, Stefano Brivio wrote:

It has nothing to do with tap_sock_unix_init(). It used to be there as that function could be called multiple times per passt instance, but it's not the case anymore.

This also takes care of the fact that, with --fd, we wouldn't set the initial MAC address, so we would need to wait for the guest to send us an ARP packet before we could exchange data.

Fixes: 6b4e68383c66 ("passt, tap: Add --fd option") Signed-off-by: Stefano Brivio --- tap.c | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/tap.c b/tap.c index 91fd2e2..177fe26 100644 --- a/tap.c +++ b/tap.c @@ -1111,12 +1111,6 @@ static void tap_sock_unix_init(struct ctx *c) if (fd < 0) die("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 - * will reach it. - */ - memset(&c->mac_guest, 0xff, sizeof(c->mac_guest)); - for (i = 1; i < UNIX_SOCK_MAX; i++) { char *path = addr.sun_path; int ex, ret; @@ -1312,6 +1306,12 @@ void tap_sock_init(struct ctx *c) if (c->mode == MODE_PASST) { if (c->fd_tap_listen == -1) tap_sock_unix_init(c); + + /* In passt mode, we don't know the guest's MAC address until it + * sends us packets. Use the broadcast address so that our + * first packets will reach it. + */ + memset(&c->mac_guest, 0xff, sizeof(c->mac_guest)); } else { tap_sock_tun_init(c); }

Reading tap.c, the effect of this is that memset will also be called when c->fd_tap_listen is set (the --fd option). As c cannot be NULL and c->mac_guest exists, this seems safe. Acked-by: Richard W.M. Jones Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com libguestfs lets you edit virtual machines. Supports shell scripting, bindings from many languages. http://libguestfs.org

On Wed, May 22, 2024 at 10:59:07PM +0200, Stefano Brivio wrote:

We'll need to open and bind the socket a while before listening to it, so split that into two different functions. No functional changes intended.

Signed-off-by: Stefano Brivio

Reviewed-by: David Gibson

--- tap.c | 39 +++++++++++++++++++++++++++------------ 1 file changed, 27 insertions(+), 12 deletions(-)

diff --git a/tap.c b/tap.c index cb6df5a..c9f580e 100644 --- a/tap.c +++ b/tap.c @@ -1095,14 +1095,14 @@ restart: }

/** - * tap_sock_unix_init() - Create and bind AF_UNIX socket, listen for connection - * @c: Execution context + * tap_sock_unix_open() - Create and bind AF_UNIX socket + * @sock_path: Socket path. If empty, set on return (UNIX_SOCK_PATH as prefix) + * + * Return: socket descriptor on success, won't return on failure */ -static void tap_sock_unix_init(struct ctx *c) +static int tap_sock_unix_open(char *sock_path) { int fd = socket(AF_UNIX, SOCK_STREAM, 0); - union epoll_ref ref = { .type = EPOLL_TYPE_TAP_LISTEN }; - struct epoll_event ev = { 0 }; struct sockaddr_un addr = { .sun_family = AF_UNIX, }; @@ -1115,8 +1115,8 @@ static void tap_sock_unix_init(struct ctx *c) char *path = addr.sun_path; int ex, ret;

- if (*c->sock_path) - memcpy(path, c->sock_path, UNIX_PATH_MAX); + if (*sock_path) + memcpy(path, sock_path, UNIX_PATH_MAX); else snprintf(path, UNIX_PATH_MAX - 1, UNIX_SOCK_PATH, i);

@@ -1127,7 +1127,7 @@ static void tap_sock_unix_init(struct ctx *c) ret = connect(ex, (const struct sockaddr *)&addr, sizeof(addr)); if (!ret || (errno != ENOENT && errno != ECONNREFUSED && errno != EACCES)) { - if (*c->sock_path) + if (*sock_path) die("Socket path %s already in use", path);

close(ex); @@ -1137,7 +1137,7 @@ static void tap_sock_unix_init(struct ctx *c)

unlink(path); if (!bind(fd, (const struct sockaddr *)&addr, sizeof(addr)) || - *c->sock_path) + *sock_path) break; }

@@ -1145,17 +1145,31 @@ static void tap_sock_unix_init(struct ctx *c) die("UNIX socket bind: %s", strerror(errno));

info("UNIX domain socket bound at %s\n", addr.sun_path); + if (!*sock_path) + memcpy(sock_path, addr.sun_path, UNIX_PATH_MAX); + + return fd; +} + +/** + * tap_sock_unix_init() - Start listening for connections on AF_UNIX socket + * @c: Execution context + */ +static void tap_sock_unix_init(struct ctx *c) +{ + union epoll_ref ref = { .type = EPOLL_TYPE_TAP_LISTEN }; + struct epoll_event ev = { 0 };

- listen(fd, 0); + listen(c->fd_tap_listen, 0);

- ref.fd = c->fd_tap_listen = fd; + ref.fd = c->fd_tap_listen; ev.events = EPOLLIN | EPOLLET; ev.data.u64 = ref.u64; epoll_ctl(c->epollfd, EPOLL_CTL_ADD, c->fd_tap_listen, &ev);

info("You can now start qemu (>= 7.2, with commit 13c6be96618c):"); info(" kvm ... -device virtio-net-pci,netdev=s -netdev stream,id=s,server=off,addr.type=unix,addr.path=%s", - addr.sun_path); + c->sock_path); info("or qrap, for earlier qemu versions:"); info(" ./qrap 5 kvm ... -net socket,fd=5 -net nic,model=virtio"); } @@ -1304,6 +1318,7 @@ void tap_sock_init(struct ctx *c) }

if (c->mode == MODE_PASST) { + c->fd_tap_listen = tap_sock_unix_open(c->sock_path); tap_sock_unix_init(c);

/* In passt mode, we don't know the guest's MAC address until it

-- 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

On Wed, May 22, 2024 at 10:59:08PM +0200, Stefano Brivio wrote:

As I'm adding pidfile_open() in the next patch. The function comment didn't match, by the way.

Signed-off-by: Stefano Brivio --- passt.c | 2 +- util.c | 6 +++--- util.h | 2 +- 3 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/passt.c b/passt.c index 1df1dc4..fb9773d 100644 --- a/passt.c +++ b/passt.c @@ -317,7 +317,7 @@ int main(int argc, char **argv) if (!c.foreground) __daemon(pidfile_fd, devnull_fd); else - write_pidfile(pidfile_fd, getpid()); + pidfile_write(pidfile_fd, getpid());

if (pasta_child_pid) kill(pasta_child_pid, SIGUSR1); diff --git a/util.c b/util.c index 849fa7f..18c04ba 100644 --- a/util.c +++ b/util.c @@ -380,11 +380,11 @@ int open_in_ns(const struct ctx *c, const char *path, int flags) }

/** - * pid_file() - Write PID to file, if requested to do so, and close it + * pidfile_write() - Write PID to file, if requested to do so, and close it * @fd: Open PID file descriptor, closed on exit, -1 to skip writing it * @pid: PID value to write */ -void write_pidfile(int fd, pid_t pid) +void pidfile_write(int fd, pid_t pid) { char pid_buf[12]; int n; @@ -419,7 +419,7 @@ int __daemon(int pidfile_fd, int devnull_fd) }

if (pid) { - write_pidfile(pidfile_fd, pid); + pidfile_write(pidfile_fd, pid); exit(EXIT_SUCCESS); }

diff --git a/util.h b/util.h index 264423b..f811975 100644 --- a/util.h +++ b/util.h @@ -156,7 +156,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); -void write_pidfile(int fd, pid_t pid); +void pidfile_write(int fd, pid_t pid); int __daemon(int pidfile_fd, int devnull_fd); int fls(unsigned long x); int write_file(const char *path, const char *buf);

Neutral refactoring, so: Reviewed-by: Richard W.M. Jones However the same function name appears (just as a comment) in contrib/apparmor/usr.bin.passt so I don't know if you would want to update that? Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com virt-top is 'top' for virtual machines. Tiny program with many powerful monitoring features, net stats, disk stats, logging, etc. http://people.redhat.com/~rjones/virt-top

On Wed, May 22, 2024 at 10:59:09PM +0200, Stefano Brivio wrote:

We won't call it from main() any longer: move it.

Signed-off-by: Stefano Brivio

Reviewed-by: David Gibson

--- passt.c | 11 ++--------- util.c | 22 ++++++++++++++++++++++ util.h | 1 + 3 files changed, 25 insertions(+), 9 deletions(-)

diff --git a/passt.c b/passt.c index fb9773d..e2446fc 100644 --- a/passt.c +++ b/passt.c @@ -199,7 +199,7 @@ void exit_handler(int signal) */ int main(int argc, char **argv) { - int nfds, i, devnull_fd = -1, pidfile_fd = -1; + int nfds, i, devnull_fd = -1, pidfile_fd; struct epoll_event events[EPOLL_EVENTS]; char *log_name, argv0[PATH_MAX], *name; struct ctx c = { 0 }; @@ -299,14 +299,7 @@ int main(int argc, char **argv) } }

- if (*c.pid_file) { - if ((pidfile_fd = open(c.pid_file, - O_CREAT | O_TRUNC | O_WRONLY | O_CLOEXEC, - S_IRUSR | S_IWUSR)) < 0) { - perror("PID file open"); - exit(EXIT_FAILURE); - } - } + pidfile_fd = pidfile_open(c.pid_file);

if (isolate_prefork(&c)) die("Failed to sandbox process, exiting"); diff --git a/util.c b/util.c index 18c04ba..cf5a62b 100644 --- a/util.c +++ b/util.c @@ -402,6 +402,28 @@ void pidfile_write(int fd, pid_t pid) close(fd); }

+/** + * pidfile_open() - Open PID file if needed + * @path: Path for PID file, empty string if no PID file is requested + * + * Return: descriptor for PID file, -1 if path is NULL, won't return on failure + */ +int pidfile_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; +} + /** * __daemon() - daemon()-like function writing PID file before parent exits * @pidfile_fd: Open PID file descriptor diff --git a/util.h b/util.h index f811975..8a430ca 100644 --- a/util.h +++ b/util.h @@ -156,6 +156,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); void pidfile_write(int fd, pid_t pid); int __daemon(int pidfile_fd, int devnull_fd); int fls(unsigned long x);

-- 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

On Wed, May 22, 2024 at 10:59:10PM +0200, Stefano Brivio wrote:

Otherwise, if the user runs us as root, and gives us paths that are only accessible by root, we'll fail to open them, which might in turn encourage users to change permissions or ownerships: definitely a bad idea in terms of security.

Reported-by: Minxi Hou Reported-by: Richard W.M. Jones Signed-off-by: Stefano Brivio --- conf.c | 17 ++++++++++++++++- passt.c | 10 ++++------ passt.h | 4 ++++ tap.c | 7 +++---- tap.h | 1 + 5 files changed, 28 insertions(+), 11 deletions(-)

diff --git a/conf.c b/conf.c index 2e0d909..f62a5eb 100644 --- a/conf.c +++ b/conf.c @@ -38,6 +38,7 @@ #include "ip.h" #include "passt.h" #include "netlink.h" +#include "tap.h" #include "udp.h" #include "tcp.h" #include "pasta.h" @@ -1093,7 +1094,7 @@ static void conf_ugid(char *runas, uid_t *uid, gid_t *gid) return;

/* ...otherwise use nobody:nobody */ - warn("Started as root. Changing to nobody..."); + warn("Started as root, will change to nobody."); { #ifndef GLIBC_NO_STATIC_NSS const struct passwd *pw; @@ -1113,6 +1114,18 @@ static void conf_ugid(char *runas, uid_t *uid, gid_t *gid) } }

+/** + * conf_open_files() - Open files as requested by configuration + * @c: Execution context + */ +static void conf_open_files(struct ctx *c) +{ + if (c->mode == MODE_PASST && c->fd_tap == -1) + c->fd_tap_listen = tap_sock_unix_open(c->sock_path); + + c->pidfile_fd = pidfile_open(c->pid_file); +} + /** * conf() - Process command-line arguments and set configuration * @c: Execution context @@ -1712,6 +1725,8 @@ void conf(struct ctx *c, int argc, char **argv) else if (optind != argc) die("Extra non-option argument: %s", argv[optind]);

+ conf_open_files(c); /* Before any possible setuid() / setgid() */ + isolate_user(uid, gid, !netns_only, userns, c->mode);

if (c->pasta_conf_ns) diff --git a/passt.c b/passt.c index e2446fc..a8c4cd3 100644 --- a/passt.c +++ b/passt.c @@ -199,9 +199,9 @@ void exit_handler(int signal) */ int main(int argc, char **argv) { - int nfds, i, devnull_fd = -1, pidfile_fd; struct epoll_event events[EPOLL_EVENTS]; char *log_name, argv0[PATH_MAX], *name; + int nfds, i, devnull_fd = -1; struct ctx c = { 0 }; struct rlimit limit; struct timespec now; @@ -211,7 +211,7 @@ int main(int argc, char **argv)

isolate_initial();

- c.pasta_netns_fd = c.fd_tap = -1; + c.pasta_netns_fd = c.fd_tap = c.pidfile_fd = -1;

sigemptyset(&sa.sa_mask); sa.sa_flags = 0; @@ -299,8 +299,6 @@ int main(int argc, char **argv) } }

- pidfile_fd = pidfile_open(c.pid_file); - if (isolate_prefork(&c)) die("Failed to sandbox process, exiting");

@@ -308,9 +306,9 @@ int main(int argc, char **argv) __openlog(log_name, 0, LOG_DAEMON);

if (!c.foreground) - __daemon(pidfile_fd, devnull_fd); + __daemon(c.pidfile_fd, devnull_fd); else - pidfile_write(pidfile_fd, getpid()); + pidfile_write(c.pidfile_fd, getpid());

if (pasta_child_pid) kill(pasta_child_pid, SIGUSR1); diff --git a/passt.h b/passt.h index bc58d64..3e50612 100644 --- a/passt.h +++ b/passt.h @@ -185,6 +185,7 @@ struct ip6_ctx { * @sock_path: Path for UNIX domain socket * @pcap: Path for packet capture file * @pid_file: Path to PID file, empty string if not configured + * @pidfile_fd: File descriptor for PID file, -1 if none * @pasta_netns_fd: File descriptor for network namespace in pasta mode * @no_netns_quit: In pasta mode, don't exit if fs-bound namespace is gone * @netns_base: Base name for fs-bound namespace, if any, in pasta mode @@ -234,7 +235,10 @@ struct ctx { int nofile; char sock_path[UNIX_PATH_MAX]; char pcap[PATH_MAX]; + char pid_file[PATH_MAX]; + int pidfile_fd; + int one_off;

int pasta_netns_fd; diff --git a/tap.c b/tap.c index c9f580e..2ea0849 100644 --- a/tap.c +++ b/tap.c @@ -1100,7 +1100,7 @@ restart: * * Return: socket descriptor on success, won't return on failure */ -static int tap_sock_unix_open(char *sock_path) +int tap_sock_unix_open(char *sock_path) { int fd = socket(AF_UNIX, SOCK_STREAM, 0); struct sockaddr_un addr = { @@ -1144,7 +1144,7 @@ static int tap_sock_unix_open(char *sock_path) if (i == UNIX_SOCK_MAX) die("UNIX socket bind: %s", strerror(errno));

- info("UNIX domain socket bound at %s\n", addr.sun_path); + info("UNIX domain socket bound at %s", addr.sun_path); if (!*sock_path) memcpy(sock_path, addr.sun_path, UNIX_PATH_MAX);

@@ -1167,7 +1167,7 @@ static void tap_sock_unix_init(struct ctx *c) ev.data.u64 = ref.u64; epoll_ctl(c->epollfd, EPOLL_CTL_ADD, c->fd_tap_listen, &ev);

- info("You can now start qemu (>= 7.2, with commit 13c6be96618c):"); + info("\nYou can now start qemu (>= 7.2, with commit 13c6be96618c):"); info(" kvm ... -device virtio-net-pci,netdev=s -netdev stream,id=s,server=off,addr.type=unix,addr.path=%s", c->sock_path); info("or qrap, for earlier qemu versions:"); @@ -1318,7 +1318,6 @@ void tap_sock_init(struct ctx *c) }

if (c->mode == MODE_PASST) { - c->fd_tap_listen = tap_sock_unix_open(c->sock_path); tap_sock_unix_init(c);

/* In passt mode, we don't know the guest's MAC address until it diff --git a/tap.h b/tap.h index d146d2f..2285a87 100644 --- a/tap.h +++ b/tap.h @@ -68,6 +68,7 @@ void tap_handler_pasta(struct ctx *c, uint32_t events, const struct timespec *now); void tap_handler_passt(struct ctx *c, uint32_t events, const struct timespec *now); +int tap_sock_unix_open(char *sock_path); void tap_sock_init(struct ctx *c);

#endif /* TAP_H */

Seems sensible. The code to open the pidfile definitely moves to the new function and the new function is called just before passt isolates itself in the new namespace. Acked-by: Richard W.M. Jones Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com virt-builder quickly builds VMs from scratch http://libguestfs.org/virt-builder.1.html

On Wed, May 29, 2024 at 12:35:24PM +1000, David Gibson wrote:

On Wed, May 22, 2024 at 10:59:10PM +0200, Stefano Brivio wrote:

...

Otherwise, if the user runs us as root, and gives us paths that are only accessible by root, we'll fail to open them, which might in turn encourage users to change permissions or ownerships: definitely a bad idea in terms of security.

Reported-by: Minxi Hou Reported-by: Richard W.M. Jones Signed-off-by: Stefano Brivio

Looking at this I did notice a pre-existing, well, maybe not bug exactly, but possibly surprising behaviour, which makes me a bit more nervous now that we can invoke it as root.

tap_sock_unix_open() will silently truncate the given socket path to the maximum length for a Unix socket. Which means we could bind(), but also unlink() a path that's not exactly the same as the one the one the user requested. I don't immediately see a way to exploit that, but it's the sort of thing that makes me nervous. I think we should instead outright fail if the given socket path is too long.

Yes, agreed. It seems as if the latest passt code still does this. Do you want me to open a bug about it? Rich.

...

--- conf.c | 17 ++++++++++++++++- passt.c | 10 ++++------ passt.h | 4 ++++ tap.c | 7 +++---- tap.h | 1 + 5 files changed, 28 insertions(+), 11 deletions(-)

diff --git a/conf.c b/conf.c index 2e0d909..f62a5eb 100644 --- a/conf.c +++ b/conf.c @@ -38,6 +38,7 @@ #include "ip.h" #include "passt.h" #include "netlink.h" +#include "tap.h" #include "udp.h" #include "tcp.h" #include "pasta.h" @@ -1093,7 +1094,7 @@ static void conf_ugid(char *runas, uid_t *uid, gid_t *gid) return;

/* ...otherwise use nobody:nobody */ - warn("Started as root. Changing to nobody..."); + warn("Started as root, will change to nobody."); { #ifndef GLIBC_NO_STATIC_NSS const struct passwd *pw; @@ -1113,6 +1114,18 @@ static void conf_ugid(char *runas, uid_t *uid, gid_t *gid) } }

+/** + * conf_open_files() - Open files as requested by configuration + * @c: Execution context + */ +static void conf_open_files(struct ctx *c) +{ + if (c->mode == MODE_PASST && c->fd_tap == -1) + c->fd_tap_listen = tap_sock_unix_open(c->sock_path); + + c->pidfile_fd = pidfile_open(c->pid_file); +} + /** * conf() - Process command-line arguments and set configuration * @c: Execution context @@ -1712,6 +1725,8 @@ void conf(struct ctx *c, int argc, char **argv) else if (optind != argc) die("Extra non-option argument: %s", argv[optind]);

+ conf_open_files(c); /* Before any possible setuid() / setgid() */ + isolate_user(uid, gid, !netns_only, userns, c->mode);

if (c->pasta_conf_ns) diff --git a/passt.c b/passt.c index e2446fc..a8c4cd3 100644 --- a/passt.c +++ b/passt.c @@ -199,9 +199,9 @@ void exit_handler(int signal) */ int main(int argc, char **argv) { - int nfds, i, devnull_fd = -1, pidfile_fd; struct epoll_event events[EPOLL_EVENTS]; char *log_name, argv0[PATH_MAX], *name; + int nfds, i, devnull_fd = -1; struct ctx c = { 0 }; struct rlimit limit; struct timespec now; @@ -211,7 +211,7 @@ int main(int argc, char **argv)

isolate_initial();

- c.pasta_netns_fd = c.fd_tap = -1; + c.pasta_netns_fd = c.fd_tap = c.pidfile_fd = -1;

sigemptyset(&sa.sa_mask); sa.sa_flags = 0; @@ -299,8 +299,6 @@ int main(int argc, char **argv) } }

- pidfile_fd = pidfile_open(c.pid_file); - if (isolate_prefork(&c)) die("Failed to sandbox process, exiting");

@@ -308,9 +306,9 @@ int main(int argc, char **argv) __openlog(log_name, 0, LOG_DAEMON);

if (!c.foreground) - __daemon(pidfile_fd, devnull_fd); + __daemon(c.pidfile_fd, devnull_fd); else - pidfile_write(pidfile_fd, getpid()); + pidfile_write(c.pidfile_fd, getpid());

if (pasta_child_pid) kill(pasta_child_pid, SIGUSR1); diff --git a/passt.h b/passt.h index bc58d64..3e50612 100644 --- a/passt.h +++ b/passt.h @@ -185,6 +185,7 @@ struct ip6_ctx { * @sock_path: Path for UNIX domain socket * @pcap: Path for packet capture file * @pid_file: Path to PID file, empty string if not configured + * @pidfile_fd: File descriptor for PID file, -1 if none * @pasta_netns_fd: File descriptor for network namespace in pasta mode * @no_netns_quit: In pasta mode, don't exit if fs-bound namespace is gone * @netns_base: Base name for fs-bound namespace, if any, in pasta mode @@ -234,7 +235,10 @@ struct ctx { int nofile; char sock_path[UNIX_PATH_MAX]; char pcap[PATH_MAX]; + char pid_file[PATH_MAX]; + int pidfile_fd; + int one_off;

int pasta_netns_fd; diff --git a/tap.c b/tap.c index c9f580e..2ea0849 100644 --- a/tap.c +++ b/tap.c @@ -1100,7 +1100,7 @@ restart: * * Return: socket descriptor on success, won't return on failure */ -static int tap_sock_unix_open(char *sock_path) +int tap_sock_unix_open(char *sock_path) { int fd = socket(AF_UNIX, SOCK_STREAM, 0); struct sockaddr_un addr = { @@ -1144,7 +1144,7 @@ static int tap_sock_unix_open(char *sock_path) if (i == UNIX_SOCK_MAX) die("UNIX socket bind: %s", strerror(errno));

- info("UNIX domain socket bound at %s\n", addr.sun_path); + info("UNIX domain socket bound at %s", addr.sun_path); if (!*sock_path) memcpy(sock_path, addr.sun_path, UNIX_PATH_MAX);

@@ -1167,7 +1167,7 @@ static void tap_sock_unix_init(struct ctx *c) ev.data.u64 = ref.u64; epoll_ctl(c->epollfd, EPOLL_CTL_ADD, c->fd_tap_listen, &ev);

- info("You can now start qemu (>= 7.2, with commit 13c6be96618c):"); + info("\nYou can now start qemu (>= 7.2, with commit 13c6be96618c):"); info(" kvm ... -device virtio-net-pci,netdev=s -netdev stream,id=s,server=off,addr.type=unix,addr.path=%s", c->sock_path); info("or qrap, for earlier qemu versions:"); @@ -1318,7 +1318,6 @@ void tap_sock_init(struct ctx *c) }

if (c->mode == MODE_PASST) { - c->fd_tap_listen = tap_sock_unix_open(c->sock_path); tap_sock_unix_init(c);

/* In passt mode, we don't know the guest's MAC address until it diff --git a/tap.h b/tap.h index d146d2f..2285a87 100644 --- a/tap.h +++ b/tap.h @@ -68,6 +68,7 @@ void tap_handler_pasta(struct ctx *c, uint32_t events, const struct timespec *now); void tap_handler_passt(struct ctx *c, uint32_t events, const struct timespec *now); +int tap_sock_unix_open(char *sock_path); void tap_sock_init(struct ctx *c);

#endif /* TAP_H */

-- 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

-- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com nbdkit - Flexible, fast NBD server with plugins https://gitlab.com/nbdkit/nbdkit

On Wed, May 22, 2024 at 10:59:11PM +0200, Stefano Brivio wrote:

We have pidfile_fd now, pid_file_fd would be quite ugly.

Signed-off-by: Stefano Brivio --- conf.c | 8 ++++---- passt.h | 4 ++-- 2 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/conf.c b/conf.c index f62a5eb..50383a3 100644 --- a/conf.c +++ b/conf.c @@ -1123,7 +1123,7 @@ static void conf_open_files(struct ctx *c) if (c->mode == MODE_PASST && c->fd_tap == -1) c->fd_tap_listen = tap_sock_unix_open(c->sock_path);

- c->pidfile_fd = pidfile_open(c->pid_file); + c->pidfile_fd = pidfile_open(c->pidfile); }

/** @@ -1456,12 +1456,12 @@ void conf(struct ctx *c, int argc, char **argv)

break; case 'P': - if (*c->pid_file) + if (*c->pidfile) die("Multiple --pid options given");

- ret = snprintf(c->pid_file, sizeof(c->pid_file), "%s", + ret = snprintf(c->pidfile, sizeof(c->pidfile), "%s", optarg); - if (ret <= 0 || ret >= (int)sizeof(c->pid_file)) + if (ret <= 0 || ret >= (int)sizeof(c->pidfile)) die("Invalid PID file: %s", optarg);

break; diff --git a/passt.h b/passt.h index 3e50612..46d073a 100644 --- a/passt.h +++ b/passt.h @@ -184,7 +184,7 @@ struct ip6_ctx { * @nofile: Maximum number of open files (ulimit -n) * @sock_path: Path for UNIX domain socket * @pcap: Path for packet capture file - * @pid_file: Path to PID file, empty string if not configured + * @pidfile: Path to PID file, empty string if not configured * @pidfile_fd: File descriptor for PID file, -1 if none * @pasta_netns_fd: File descriptor for network namespace in pasta mode * @no_netns_quit: In pasta mode, don't exit if fs-bound namespace is gone @@ -236,7 +236,7 @@ struct ctx { char sock_path[UNIX_PATH_MAX]; char pcap[PATH_MAX];

- char pid_file[PATH_MAX]; + char pidfile[PATH_MAX]; int pidfile_fd;

int one_off;

Looks like trivial refactor so: Reviewed-by: Richard W.M. Jones Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com libguestfs lets you edit virtual machines. Supports shell scripting, bindings from many languages. http://libguestfs.org