* [Qemu-devel] [PATCH v2] qemu-nbd: Implement socket activation. @ 2017-02-02 17:16 Richard W.M. Jones 2017-02-02 17:16 ` Richard W.M. Jones 0 siblings, 1 reply; 6+ messages in thread From: Richard W.M. Jones @ 2017-02-02 17:16 UTC (permalink / raw) To: pbonzini; +Cc: qemu-devel, den, rkagan, dplotnikov, berrange, stefanha v2: - A few small fixed identified by Dan Berrange. The original cover letter is below. Rich. Socket activation (sometimes known as systemd socket activation) allows an Internet superserver to pass a pre-opened listening socket to the process, instead of having qemu-nbd open a socket itself. This is done via the LISTEN_FDS and LISTEN_PID environment variables, and a standard file descriptor range. This patch partially implements socket activation. The limitation of this implementation is that qemu-nbd can only listen on a single file descriptor, and so if LISTEN_FDS > 1 (eg. for listening on multiple interfaces or ports) socket activation will fail. However for the simple case of listening on a single port, and either all interfaces with IPv4+IPv6, or just a loopback interface, the current implementation works fine. Fixing this properly would require considerable changes throughout qemu, since qemu's currently handling of getaddrinfo is plainly wrong. To use qemu-nbd from systemd, you create /etc/systemd/system/nbd.socket: [Unit] Description=QEMU Network Block Device server [Socket] ListenStream=10809 [Install] WantedBy=sockets.target and /etc/systemd/system/nbd.service: [Service] ExecStart=/usr/sbin/qemu-nbd -v -t /path/to/file and enable the socket service (only): systemctl enable nbd.socket systemctl start nbd.socket and then connecting to port 10809 will start qemu-nbd and service the file, with systemd opening the listening socket. In the ExecStart line, the qemu-nbd -v option is only needed if you want enhanced debugging. The -t option is required unless you want to fiddle with systemd settings for rate-limiting. If you try to use the -p and similar options with socket activation then qemu-nbd will give an error. (I wasn't sure where to document this -- there is no obvious documentation for qemu-nbd beyond the simple list of command line arguments) This is based on the implementations in libvirt (src/util/virutil.c:virGetListenFDs) and nbdkit (src/main.c:get_socket_activation), and also on Denis Plotnikov's implementation of --server-sock-fd (https://lists.gnu.org/archive/html/qemu-devel/2016-09/msg07781.html). ^ permalink raw reply [flat|nested] 6+ messages in thread
* [Qemu-devel] [PATCH v2] qemu-nbd: Implement socket activation. 2017-02-02 17:16 [Qemu-devel] [PATCH v2] qemu-nbd: Implement socket activation Richard W.M. Jones @ 2017-02-02 17:16 ` Richard W.M. Jones 2017-02-02 17:30 ` Daniel P. Berrange 2017-02-03 15:16 ` Stefan Hajnoczi 0 siblings, 2 replies; 6+ messages in thread From: Richard W.M. Jones @ 2017-02-02 17:16 UTC (permalink / raw) To: pbonzini; +Cc: qemu-devel, den, rkagan, dplotnikov, berrange, stefanha Socket activation (sometimes known as systemd socket activation) allows an Internet superserver to pass a pre-opened listening socket to the process, instead of having qemu-nbd open a socket itself. This is done via the LISTEN_FDS and LISTEN_PID environment variables, and a standard file descriptor range. This change partially implements socket activation for qemu-nbd. If the environment variables are set correctly, then socket activation will happen automatically, otherwise everything works as before. The limitation is that LISTEN_FDS must be 1. Signed-off-by: Richard W.M. Jones. --- qemu-nbd.c | 172 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++---- 1 file changed, 163 insertions(+), 9 deletions(-) diff --git a/qemu-nbd.c b/qemu-nbd.c index c734f62..b3088d0 100644 --- a/qemu-nbd.c +++ b/qemu-nbd.c @@ -463,6 +463,135 @@ static QCryptoTLSCreds *nbd_get_tls_creds(const char *id, Error **errp) return creds; } +static void setup_address_and_port(const char **address, const char **port) +{ + if (*address == NULL) { + *address = "0.0.0.0"; + } + + if (*port == NULL) { + *port = g_strdup_printf("%d", NBD_DEFAULT_PORT);; + } +} + +#define FIRST_SOCKET_ACTIVATION_FD 3 /* defined by systemd ABI */ + +#ifndef _WIN32 +/* + * Check if socket activation was requested via use of the + * LISTEN_FDS and LISTEN_PID environment variables. + * + * Returns 0 if no socket activation, or the number of FDs. + */ +static unsigned int check_socket_activation(void) +{ + const char *s; + unsigned long pid; + unsigned long nr_fds; + unsigned int i; + int fd; + int err; + + s = getenv("LISTEN_PID"); + if (s == NULL) { + return 0; + } + err = qemu_strtoul(s, NULL, 10, &pid); + if (err) { + if (verbose) { + fprintf(stderr, "malformed %s environment variable (ignored)\n", + "LISTEN_PID"); + } + return 0; + } + if (pid != getpid()) { + if (verbose) { + fprintf(stderr, "%s was not for us (ignored)\n", + "LISTEN_PID"); + } + return 0; + } + + s = getenv("LISTEN_FDS"); + if (s == NULL) { + return 0; + } + err = qemu_strtoul(s, NULL, 10, &nr_fds); + if (err) { + if (verbose) { + fprintf(stderr, "malformed %s environment variable (ignored)\n", + "LISTEN_FDS"); + } + return 0; + } + assert(nr_fds <= UINT_MAX); + + /* A limitation of current qemu-nbd is that it can only listen on + * a single socket. When that limitation is lifted, we can change + * this function to allow LISTEN_FDS > 1, and remove the assertion + * in the main function below. + */ + if (nr_fds > 1) { + error_report("qemu-nbd does not support socket activation with %s > 1", + "LISTEN_FDS"); + exit(EXIT_FAILURE); + } + + /* So these are not passed to any child processes we might start. */ + unsetenv("LISTEN_FDS"); + unsetenv("LISTEN_PID"); + + /* So the file descriptors don't leak into child processes. */ + for (i = 0; i < nr_fds; ++i) { + fd = FIRST_SOCKET_ACTIVATION_FD + i; + if (fcntl(fd, F_SETFD, FD_CLOEXEC) == -1) { + /* If we cannot set FD_CLOEXEC then it probably means the file + * descriptor is invalid, so socket activation has gone wrong + * and we should exit. + */ + error_report("Socket activation failed: " + "invalid file descriptor fd = %d: %m", + fd); + exit(EXIT_FAILURE); + } + } + + return (unsigned int) nr_fds; +} + +#else /* !_WIN32 */ +static unsigned int check_socket_activation(void) +{ + return 0; +} +#endif + +/* + * Check socket parameters compatibility when socket activation is used. + */ +static const char *socket_activation_validate_opts(const char *device, + const char *sockpath, + const char *address, + const char *port) +{ + if (device != NULL) { + return "NBD device can't be set when using socket activation"; + } + + if (sockpath != NULL) { + return "Unix socket can't be set when using socket activation"; + } + + if (address != NULL) { + return "The interface can't be set when using socket activation"; + } + + if (port != NULL) { + return "TCP port number can't be set when using socket activation"; + } + + return NULL; +} int main(int argc, char **argv) { @@ -471,7 +600,7 @@ int main(int argc, char **argv) off_t dev_offset = 0; uint16_t nbdflags = 0; bool disconnect = false; - const char *bindto = "0.0.0.0"; + const char *bindto = NULL; const char *port = NULL; char *sockpath = NULL; char *device = NULL; @@ -533,6 +662,7 @@ int main(int argc, char **argv) char *trace_file = NULL; bool fork_process = false; int old_stderr = -1; + unsigned socket_activation; /* The client thread uses SIGTERM to interrupt the server. A signal * handler ensures that "qemu-nbd -v -c" exits with a nice status code. @@ -751,6 +881,19 @@ int main(int argc, char **argv) trace_init_file(trace_file); qemu_set_log(LOG_TRACE); + socket_activation = check_socket_activation(); + if (socket_activation == 0) { + setup_address_and_port(&bindto, &port); + } else { + /* Using socket activation - check user didn't use -p etc. */ + const char *err_msg = socket_activation_validate_opts(device, sockpath, + bindto, port); + if (err_msg != NULL) { + error_report("%s", err_msg); + exit(EXIT_FAILURE); + } + } + if (tlscredsid) { if (sockpath) { error_report("TLS is only supported with IPv4/IPv6"); @@ -855,7 +998,25 @@ int main(int argc, char **argv) snprintf(sockpath, 128, SOCKET_PATH, basename(device)); } - saddr = nbd_build_socket_address(sockpath, bindto, port); + if (socket_activation == 0) { + server_ioc = qio_channel_socket_new(); + saddr = nbd_build_socket_address(sockpath, bindto, port); + if (qio_channel_socket_listen_sync(server_ioc, saddr, &local_err) < 0) { + object_unref(OBJECT(server_ioc)); + error_report_err(local_err); + return 1; + } + } else { + /* See comment in check_socket_activation above. */ + assert(socket_activation == 1); + server_ioc = qio_channel_socket_new_fd(FIRST_SOCKET_ACTIVATION_FD, + &local_err); + if (server_ioc == NULL) { + error_report("Failed to use socket activation: %s", + error_get_pretty(local_err)); + exit(EXIT_FAILURE); + } + } if (qemu_init_main_loop(&local_err)) { error_report_err(local_err); @@ -950,13 +1111,6 @@ int main(int argc, char **argv) exit(EXIT_FAILURE); } - server_ioc = qio_channel_socket_new(); - if (qio_channel_socket_listen_sync(server_ioc, saddr, &local_err) < 0) { - object_unref(OBJECT(server_ioc)); - error_report_err(local_err); - return 1; - } - if (device) { int ret; -- 2.10.2 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [Qemu-devel] [PATCH v2] qemu-nbd: Implement socket activation. 2017-02-02 17:16 ` Richard W.M. Jones @ 2017-02-02 17:30 ` Daniel P. Berrange 2017-02-03 15:16 ` Stefan Hajnoczi 1 sibling, 0 replies; 6+ messages in thread From: Daniel P. Berrange @ 2017-02-02 17:30 UTC (permalink / raw) To: Richard W.M. Jones Cc: pbonzini, qemu-devel, den, rkagan, dplotnikov, stefanha On Thu, Feb 02, 2017 at 05:16:25PM +0000, Richard W.M. Jones wrote: > Socket activation (sometimes known as systemd socket activation) > allows an Internet superserver to pass a pre-opened listening socket > to the process, instead of having qemu-nbd open a socket itself. This > is done via the LISTEN_FDS and LISTEN_PID environment variables, and a > standard file descriptor range. > > This change partially implements socket activation for qemu-nbd. If > the environment variables are set correctly, then socket activation > will happen automatically, otherwise everything works as before. The > limitation is that LISTEN_FDS must be 1. > > Signed-off-by: Richard W.M. Jones. > --- > qemu-nbd.c | 172 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++---- > 1 file changed, 163 insertions(+), 9 deletions(-) Reviewed-by: Daniel P. Berrange <berrange@redhat.com> Regards, Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://entangle-photo.org -o- http://search.cpan.org/~danberr/ :| ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Qemu-devel] [PATCH v2] qemu-nbd: Implement socket activation. 2017-02-02 17:16 ` Richard W.M. Jones 2017-02-02 17:30 ` Daniel P. Berrange @ 2017-02-03 15:16 ` Stefan Hajnoczi 2017-02-03 16:52 ` Richard W.M. Jones 1 sibling, 1 reply; 6+ messages in thread From: Stefan Hajnoczi @ 2017-02-03 15:16 UTC (permalink / raw) To: Richard W.M. Jones Cc: pbonzini, qemu-devel, den, rkagan, dplotnikov, berrange [-- Attachment #1: Type: text/plain, Size: 2166 bytes --] On Thu, Feb 02, 2017 at 05:16:25PM +0000, Richard W.M. Jones wrote: > Socket activation (sometimes known as systemd socket activation) > allows an Internet superserver to pass a pre-opened listening socket > to the process, instead of having qemu-nbd open a socket itself. This > is done via the LISTEN_FDS and LISTEN_PID environment variables, and a > standard file descriptor range. > > This change partially implements socket activation for qemu-nbd. If > the environment variables are set correctly, then socket activation > will happen automatically, otherwise everything works as before. The > limitation is that LISTEN_FDS must be 1. > > Signed-off-by: Richard W.M. Jones. > --- > qemu-nbd.c | 172 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++---- > 1 file changed, 163 insertions(+), 9 deletions(-) > > diff --git a/qemu-nbd.c b/qemu-nbd.c > index c734f62..b3088d0 100644 > --- a/qemu-nbd.c > +++ b/qemu-nbd.c > @@ -463,6 +463,135 @@ static QCryptoTLSCreds *nbd_get_tls_creds(const char *id, Error **errp) > return creds; > } > > +static void setup_address_and_port(const char **address, const char **port) > +{ > + if (*address == NULL) { > + *address = "0.0.0.0"; > + } > + > + if (*port == NULL) { > + *port = g_strdup_printf("%d", NBD_DEFAULT_PORT);; Please stringify NBD_DEFAULT_PORT instead of using g_strdup_printf(). That avoids the memory leak. > @@ -751,6 +881,19 @@ int main(int argc, char **argv) > trace_init_file(trace_file); > qemu_set_log(LOG_TRACE); > > + socket_activation = check_socket_activation(); > + if (socket_activation == 0) { > + setup_address_and_port(&bindto, &port); > + } else { > + /* Using socket activation - check user didn't use -p etc. */ > + const char *err_msg = socket_activation_validate_opts(device, sockpath, > + bindto, port); Daemonizing is also incompatible with socket activation. We've marked the fds O_CLOEXEC so the child won't have access to them. Please add an error in case a user tries this. [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 455 bytes --] ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Qemu-devel] [PATCH v2] qemu-nbd: Implement socket activation. 2017-02-03 15:16 ` Stefan Hajnoczi @ 2017-02-03 16:52 ` Richard W.M. Jones 2017-02-04 9:58 ` Markus Armbruster 0 siblings, 1 reply; 6+ messages in thread From: Richard W.M. Jones @ 2017-02-03 16:52 UTC (permalink / raw) To: Stefan Hajnoczi; +Cc: pbonzini, qemu-devel, den, rkagan, dplotnikov, berrange On Fri, Feb 03, 2017 at 03:16:43PM +0000, Stefan Hajnoczi wrote: > On Thu, Feb 02, 2017 at 05:16:25PM +0000, Richard W.M. Jones wrote: > > + if (*port == NULL) { > > + *port = g_strdup_printf("%d", NBD_DEFAULT_PORT);; > > Please stringify NBD_DEFAULT_PORT instead of using g_strdup_printf(). > That avoids the memory leak. Oops. Do we have a macro for this already? I couldn't see one, and the best I could come up with is: #define MACRO_EXPAND_STRINGIFY(x) STRINGIFY(x) #define STRINGIFY(x) #x static void setup_address_and_port(const char **address, const char **port) { if (*address == NULL) { *address = "0.0.0.0"; } if (*port == NULL) { *port = MACRO_EXPAND_STRINGIFY(NBD_DEFAULT_PORT); } } It works, but it's a bit of a mouthful. Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com virt-p2v converts physical machines to virtual machines. Boot with a live CD or over the network (PXE) and turn machines into KVM guests. http://libguestfs.org/virt-v2v ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Qemu-devel] [PATCH v2] qemu-nbd: Implement socket activation. 2017-02-03 16:52 ` Richard W.M. Jones @ 2017-02-04 9:58 ` Markus Armbruster 0 siblings, 0 replies; 6+ messages in thread From: Markus Armbruster @ 2017-02-04 9:58 UTC (permalink / raw) To: Richard W.M. Jones Cc: Stefan Hajnoczi, qemu-devel, dplotnikov, rkagan, den, pbonzini "Richard W.M. Jones" <rjones@redhat.com> writes: > On Fri, Feb 03, 2017 at 03:16:43PM +0000, Stefan Hajnoczi wrote: >> On Thu, Feb 02, 2017 at 05:16:25PM +0000, Richard W.M. Jones wrote: >> > + if (*port == NULL) { >> > + *port = g_strdup_printf("%d", NBD_DEFAULT_PORT);; >> >> Please stringify NBD_DEFAULT_PORT instead of using g_strdup_printf(). >> That avoids the memory leak. > > Oops. > > Do we have a macro for this already? I couldn't see one, and the > best I could come up with is: > > #define MACRO_EXPAND_STRINGIFY(x) STRINGIFY(x) > #define STRINGIFY(x) #x Check out stringify() in compiler.h. (Yes, lower-case macros that don't behave like functions are bad style) [...] ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2017-02-04 9:58 UTC | newest] Thread overview: 6+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2017-02-02 17:16 [Qemu-devel] [PATCH v2] qemu-nbd: Implement socket activation Richard W.M. Jones 2017-02-02 17:16 ` Richard W.M. Jones 2017-02-02 17:30 ` Daniel P. Berrange 2017-02-03 15:16 ` Stefan Hajnoczi 2017-02-03 16:52 ` Richard W.M. Jones 2017-02-04 9:58 ` Markus Armbruster
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 NNTP newsgroup(s).