Some small refactorings which should not affect the code: - Use string_vector_reserve instead of checking each time we append. - Get rid of the hard-coded length, and use strncmp (..., s, strlen (s)). The compiler should compile this to the same code. --- generator/states-connect-socket-activation.c | 23 +++++++------------- 1 file changed, 8 insertions(+), 15 deletions(-) diff --git a/generator/states-connect-socket-activation.c b/generator/states-connect-socket-activation.c index 9a83834915..24544018fb 100644 --- a/generator/states-connect-socket-activation.c +++ b/generator/states-connect-socket-activation.c @@ -34,9 +34,6 @@ /* This is baked into the systemd socket activation API. */ #define FIRST_SOCKET_ACTIVATION_FD 3 -/* == strlen ("LISTEN_PID=") | strlen ("LISTEN_FDS=") */ -#define PREFIX_LENGTH 11 - extern char **environ; /* Prepare environment for calling execvp when doing systemd socket @@ -53,26 +50,22 @@ prepare_socket_activation_environment (string_vector *env) assert (env->len == 0); - /* Reserve slots env[0] and env[1]. */ + /* Reserve slots env[0]..env[1] */ + if (string_vector_reserve (env, 2) == -1) + goto err; p = strdup ("LISTEN_PID=xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx"); if (p == NULL) goto err; - if (string_vector_append (env, p) == -1) { - free (p); - goto err; - } + string_vector_append (env, p); p = strdup ("LISTEN_FDS=1"); if (p == NULL) goto err; - if (string_vector_append (env, p) == -1) { - free (p); - goto err; - } + string_vector_append (env, p); /* Append the current environment, but remove LISTEN_PID, LISTEN_FDS. */ for (i = 0; environ[i] != NULL; ++i) { - if (strncmp (environ[i], "LISTEN_PID=", PREFIX_LENGTH) != 0 && - strncmp (environ[i], "LISTEN_FDS=", PREFIX_LENGTH) != 0) { + if (strncmp (environ[i], "LISTEN_PID=", strlen ("LISTEN_PID=")) != 0 && + strncmp (environ[i], "LISTEN_FDS=", strlen ("LISTEN_FDS=")) != 0) { char *copy = strdup (environ[i]); if (copy == NULL) goto err; @@ -194,7 +187,7 @@ CONNECT_SA.START: char buf[32]; const char *v = nbd_internal_fork_safe_itoa ((long) getpid (), buf, sizeof buf); - strcpy (&env.ptr[0][PREFIX_LENGTH], v); + strcpy (&env.ptr[0][strlen ("LISTEN_FDS=")], v); /* Restore SIGPIPE back to SIG_DFL. */ signal (SIGPIPE, SIG_DFL); -- 2.39.0

--- .gitignore | 1 + common/include/Makefile.am | 6 +++ common/include/ascii-ctype.h | 75 ++++++++++++++++++++++++++ common/include/test-ascii-ctype.c | 88 +++++++++++++++++++++++++++++++ 4 files changed, 170 insertions(+) diff --git a/.gitignore b/.gitignore index f42737131d..90eeadb793 100644 --- a/.gitignore +++ b/.gitignore @@ -34,6 +34,7 @@ Makefile.in /bash-completion/nbdinfo /bash-completion/nbdublk /common/include/test-array-size +/common/include/test-ascii-ctype /common/include/test-checked-overflow /common/include/test-ispowerof2 /common/utils/test-human-size diff --git a/common/include/Makefile.am b/common/include/Makefile.am index fa2633c25a..04553a367d 100644 --- a/common/include/Makefile.am +++ b/common/include/Makefile.am @@ -20,6 +20,7 @@ include $(top_srcdir)/subdir-rules.mk EXTRA_DIST = \ ansi-colours.h \ array-size.h \ + ascii-ctype.h \ byte-swapping.h \ checked-overflow.h \ compiler-macros.h \ @@ -35,6 +36,7 @@ EXTRA_DIST = \ TESTS = \ test-array-size \ + test-ascii-ctype \ test-checked-overflow \ test-ispowerof2 \ $(NULL) @@ -44,6 +46,10 @@ test_array_size_SOURCES = test-array-size.c array-size.h test_array_size_CPPFLAGS = -I$(srcdir) test_array_size_CFLAGS = $(WARNINGS_CFLAGS) +test_ascii_ctype_SOURCES = test-ascii-ctype.c ascii-ctype.h +test_ascii_ctype_CPPFLAGS = -I$(srcdir) +test_ascii_ctype_CFLAGS = $(WARNINGS_CFLAGS) + test_checked_overflow_SOURCES = test-checked-overflow.c checked-overflow.h test_checked_overflow_CPPFLAGS = -I$(srcdir) test_checked_overflow_CFLAGS = $(WARNINGS_CFLAGS) diff --git a/common/include/ascii-ctype.h b/common/include/ascii-ctype.h new file mode 100644 index 0000000000..a55dee11b8 --- /dev/null +++ b/common/include/ascii-ctype.h @@ -0,0 +1,75 @@ +/* nbdkit + * Copyright (C) 2013-2020 Red Hat Inc. + * + * Redistribution and use in source and binary forms, with or without + * modification, are permitted provided that the following conditions are + * met: + * + * * Redistributions of source code must retain the above copyright + * notice, this list of conditions and the following disclaimer. + * + * * Redistributions in binary form must reproduce the above copyright + * notice, this list of conditions and the following disclaimer in the + * documentation and/or other materials provided with the distribution. + * + * * Neither the name of Red Hat nor the names of its contributors may be + * used to endorse or promote products derived from this software without + * specific prior written permission. + * + * THIS SOFTWARE IS PROVIDED BY RED HAT AND CONTRIBUTORS ''AS IS'' AND + * ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, + * THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A + * PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL RED HAT OR + * CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, + * SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT + * LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF + * USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND + * ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, + * OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT + * OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF + * SUCH DAMAGE. + */ + +/* Normal ctype functions are affected by the current locale. For + * example isupper() might recognize Ä in some but not all locales. + * These functions match only 7 bit ASCII characters. + */ + +#ifndef NBDKIT_ASCII_CTYPE_H +#define NBDKIT_ASCII_CTYPE_H + +#define ascii_isalnum(c) (ascii_isalpha (c) || ascii_isdigit (c)) + +#define ascii_isalpha(c) \ + (((c) >= 'a' && (c) <= 'z') || ((c) >= 'A' && (c) <= 'Z')) + +#define ascii_isdigit(c) \ + ((c) >= '0' && (c) <= '9') + +#define ascii_isspace(c) \ + ((c) == '\t' || (c) == '\n' || (c) == '\f' || (c) == '\r' || (c) == ' ') + +#define ascii_isupper(c) \ + ((c) >= 'A' && (c) <= 'Z') + +#define ascii_islower(c) \ + ((c) >= 'a' && (c) <= 'z') + +/* See also hexdigit.h */ +#define ascii_isxdigit(c) \ + ((c) == '0' || (c) == '1' || (c) == '2' || (c) == '3' || (c) == '4' || \ + (c) == '5' || (c) == '6' || (c) == '7' || (c) == '8' || (c) == '9' || \ + (c) == 'a' || (c) == 'b' || (c) == 'c' || \ + (c) == 'd' || (c) == 'e' || (c) == 'f' || \ + (c) == 'A' || (c) == 'B' || (c) == 'C' || \ + (c) == 'D' || (c) == 'E' || (c) == 'F') + +#define ascii_tolower(c) \ + (ascii_isupper ((c)) ? (c) + 32 : (c)) + +#define ascii_toupper(c) \ + (ascii_islower ((c)) ? (c) - 32 : (c)) + +#define ascii_isprint(c) ((c) >= 32 && (c) <= 126) + +#endif /* NBDKIT_ASCII_CTYPE_H */ diff --git a/common/include/test-ascii-ctype.c b/common/include/test-ascii-ctype.c new file mode 100644 index 0000000000..4fbb0259f8 --- /dev/null +++ b/common/include/test-ascii-ctype.c @@ -0,0 +1,88 @@ +/* nbdkit + * Copyright (C) 2020 Red Hat Inc. + * + * Redistribution and use in source and binary forms, with or without + * modification, are permitted provided that the following conditions are + * met: + * + * * Redistributions of source code must retain the above copyright + * notice, this list of conditions and the following disclaimer. + * + * * Redistributions in binary form must reproduce the above copyright + * notice, this list of conditions and the following disclaimer in the + * documentation and/or other materials provided with the distribution. + * + * * Neither the name of Red Hat nor the names of its contributors may be + * used to endorse or promote products derived from this software without + * specific prior written permission. + * + * THIS SOFTWARE IS PROVIDED BY RED HAT AND CONTRIBUTORS ''AS IS'' AND + * ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, + * THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A + * PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL RED HAT OR + * CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, + * SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT + * LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF + * USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND + * ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, + * OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT + * OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF + * SUCH DAMAGE. + */ + +#include <config.h> + +#include <stdio.h> +#include <stdlib.h> +#undef NDEBUG /* Keep test strong even for nbdkit built without assertions */ +#include <assert.h> + +#include "ascii-ctype.h" + +int +main (void) +{ + assert (ascii_isspace (' ')); + assert (ascii_isspace ('\t')); + assert (ascii_isspace ('\n')); + assert (! ascii_isspace ('a')); + + assert (ascii_isalpha ('a')); + assert (ascii_isalpha ('Z')); + assert (ascii_isalpha ('z')); + assert (! ascii_isalpha (' ')); + assert (! ascii_isalpha ('0')); + { const char *s = "Ä"; assert (! ascii_isalpha (s[0])); } + { const char *s = "®"; assert (! ascii_isalpha (s[0])); } + + assert (ascii_isdigit ('0')); + assert (ascii_isdigit ('9')); + { const char *s = "Ø"; assert (! ascii_isdigit (s[0])); } /* U+00D8 */ + { const char *s = "９"; assert (! ascii_isdigit (s[0])); } /* U+FF19 */ + + assert (ascii_islower ('a')); + assert (ascii_islower ('z')); + assert (! ascii_islower ('Z')); + { const char *s = "Ä"; assert (! ascii_islower (s[0])); } + + assert (ascii_isupper ('A')); + assert (ascii_isupper ('Z')); + assert (! ascii_isupper ('z')); + { const char *s = "Ä"; assert (! ascii_isupper (s[0])); } + + assert (ascii_tolower ('A') == 'a'); + assert (ascii_tolower ('Z') == 'z'); + assert (ascii_tolower ('a') == 'a'); + assert (ascii_tolower ('z') == 'z'); + assert (ascii_tolower ('0') == '0'); + { const char *s = "Ä"; assert (ascii_tolower (s[0]) == s[0]); } + + assert (ascii_toupper ('a') == 'A'); + assert (ascii_toupper ('z') == 'Z'); + assert (ascii_toupper ('A') == 'A'); + assert (ascii_toupper ('Z') == 'Z'); + assert (ascii_toupper ('0') == '0'); + { const char *s = "à"; assert (ascii_toupper (s[0]) == s[0]); } + + exit (EXIT_SUCCESS); +} -- 2.39.0

To allow us to name the socket passed down to the NBD server when calling nbd_connect_systemd_socket_activation(3), we need to add the field to the handle and add access functions. --- generator/API.ml | 49 ++++++++++++++++++++++++++++++++++++++++++ lib/handle.c | 56 ++++++++++++++++++++++++++++++++++++++++++++++++ lib/internal.h | 1 + 3 files changed, 106 insertions(+) diff --git a/generator/API.ml b/generator/API.ml index 25a612a2e8..08fc80960b 100644 --- a/generator/API.ml +++ b/generator/API.ml @@ -2036,15 +2036,62 @@ "connect_systemd_socket_activation", { When the NBD handle is closed the server subprocess is killed. + +=head3 Socket name + +The socket activation protocol lets you optionally give +the socket a name. If used, the name is passed to the +NBD server using the C<LISTEN_FDNAMES> environment +variable. To provide a socket name, call +L<nbd_set_socket_activation_name(3)> before calling +the connect function. " ^ blocking_connect_call_description; see_also = [Link "aio_connect_systemd_socket_activation"; Link "connect_command"; Link "kill_subprocess"; Link "set_opt_mode"; + Link "set_socket_activation_name"; + Link "get_socket_activation_name"; ExternalLink ("qemu-nbd", 1); URLLink "http://0pointer.de/blog/projects/socket-activation.html"]; example = Some "examples/open-qcow2.c"; }; + "set_socket_activation_name", { + default_call with + args = [ String "socket_name" ]; ret = RErr; + shortdesc = "set the socket activation name"; + longdesc = "\ +When running an NBD server using +L<nbd_connect_systemd_socket_activation(3)> you can optionally +name the socket. Call this function before connecting to the +server. + +Some servers such as L<qemu-storage-daemon(1)> +can use this information to associate the socket with a name +used on the command line, but most servers will ignore it. +The name is passed through the C<LISTEN_FDNAMES> environment +variable. + +The parameter C<socket_name> can be a short alphanumeric string. +If it is set to the empty string (also the default when the handle +is created) then no name is passed to the server."; + see_also = [Link "connect_systemd_socket_activation"; + Link "get_socket_activation_name"]; + }; + + "get_socket_activation_name", { + default_call with + args = []; ret = RString; + shortdesc = "get the socket activation name"; + longdesc = "\ +Return the socket name used when you call +L<nbd_connect_systemd_socket_activation(3)> on the same +handle. By default this will return the empty string +meaning that no name is passed to the server."; + see_also = [Link "connect_systemd_socket_activation"; + Link "set_socket_activation_name"]; + }; + "is_read_only", { default_call with args = []; ret = RBool; @@ -3844,6 +3891,8 @@ let first_version = "aio_opt_structured_reply", (1, 16); "opt_starttls", (1, 16); "aio_opt_starttls", (1, 16); + "set_socket_activation_name", (1, 16); + "get_socket_activation_name", (1, 16); (* These calls are proposed for a future version of libnbd, but * have not been added to any released version so far. diff --git a/lib/handle.c b/lib/handle.c index 4a186f8fa9..96c8b1f2b1 100644 --- a/lib/handle.c +++ b/lib/handle.c @@ -28,6 +28,7 @@ #include <sys/types.h> #include <sys/wait.h> +#include "ascii-ctype.h" #include "internal.h" static void @@ -161,6 +162,7 @@ nbd_close (struct nbd_handle *h) waitpid (h->pid, NULL, 0); free (h->export_name); + free (h->sa_name); free (h->tls_certificates); free (h->tls_username); free (h->tls_psk_file); @@ -200,6 +202,60 @@ nbd_unlocked_get_handle_name (struct nbd_handle *h) return copy; } +int +nbd_unlocked_set_socket_activation_name (struct nbd_handle *h, + const char *name) +{ + size_t i, len; + char *new_name; + + len = strlen (name); + + /* Setting it to empty string stores NULL in the handle. */ + if (len == 0) { + free (h->sa_name); + h->sa_name = NULL; + return 0; + } + + /* Check the proposed name is short and alphanumeric. */ + if (len > 32) { + set_error (ENAMETOOLONG, "socket activation name should be " + "<= 32 characters"); + return -1; + } + for (i = 0; i < len; ++i) { + if (! ascii_isalnum (name[i])) { + set_error (EINVAL, "socket activation name should contain " + "only alphanumeric ASCII characters"); + return -1; + } + } + + new_name = strdup (name); + if (!new_name) { + set_error (errno, "strdup"); + return -1; + } + + free (h->sa_name); + h->sa_name = new_name; + return 0; +} + +char * +nbd_unlocked_get_socket_activation_name (struct nbd_handle *h) +{ + char *copy = strdup (h->sa_name ? h->sa_name : ""); + + if (!copy) { + set_error (errno, "strdup"); + return NULL; + } + + return copy; +} + uintptr_t nbd_unlocked_set_private_data (struct nbd_handle *h, uintptr_t data) { diff --git a/lib/internal.h b/lib/internal.h index bbbd26393f..19d7f0af60 100644 --- a/lib/internal.h +++ b/lib/internal.h @@ -101,6 +101,7 @@ struct nbd_handle { _Atomic uintptr_t private_data; char *export_name; /* Export name, never NULL. */ + char *sa_name; /* Socket activation name, can be NULL. */ /* TLS settings. */ int tls; /* 0 = disable, 1 = enable, 2 = require */ -- 2.39.0

On Tue, Jan 31, 2023 at 10:29:00AM -0600, Eric Blake wrote: While I'm not totally opposed to this, I would say a few things: - the API is already stateful, even used in the most basic way, eg you must connect before you can do other things - having the state modified by various nbd_set_* calls allows us to easily add more variants, instead of having to create a combinatorial future set of nbd_connect_systemd_*_socket_activation calls So I would lean towards my design. (Also it's the same thing we already do, eg. for opt mode). I didn't know about this documentation before. Arbitrary ASCII characters doesn't sound that great though. I'm sure that q-s-d will want its own much more strict limitations, eg. I assume you can't possibly support any characters which are meaningful to JSON / QMP. Any thoughts on that? 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 1/30/23 23:55, Richard W.M. Jones wrote: "lib/handle.c" includes <signal.h>, and <signal.h> reserves symbols starting with the prefix "sa_": https://pubs.opengroup.org/onlinepubs/9699919799/functions/V2_chap02.html... While the fields of unions and structures live in a separate namespace, that's no protection from macros. Otherwise the patch looks OK to me. (I'm noticing we already have fields called "sa_tmpdir" and "sa_sockpath"; that's not ideal, although it may not matter in practice.) Reviewed-by: Laszlo Ersek <lersek(a)redhat.com&gt; Laszlo

On Mon, Jan 30, 2023 at 10:55:21PM +0000, Richard W.M. Jones wrote: Hmm. Even if we _don't_ expose the ability to set LISTEN_FDNAMES to users, we should probably be stripping it from the environment (without replacement), rather than just stripping the other two LISTEN_* variables. Which might be worth doing in a separate patch, in case it has to be backported across different versions of libnbd. But overall, I agree with exposing the ability for the client to programatically set the name they want, whether by this series or by my idea of an alternative API that takes the socket name alongside the argv; and whether we keep to our 32-byte [[:alnum:]] limit or instead allow for a larger name up to 255 bytes in the regex range notation by ASCII byte value [\x20-\x39\x41-\x7e] (aka [ -9;-~], or [^\x00-\x1f\x7f-\xff]). -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org

On 1/30/23 23:55, Richard W.M. Jones wrote: As long as we don't use a table+loop for the special variables, this seems OK to me. (For patch#1, I meant to add: wherever we know string_vector_append() can't fail, due to the reservation at the top, adding an assert() could improve readability; the reservation can get visually quite far from the repeated string_vector_append() calls.) Reviewed-by: Laszlo Ersek <lersek(a)redhat.com&gt;