[Libguestfs] [PATCH libnbd 2/5] lib: Replace a few uses of realloc with nbdkit vector library.

This allows us to remove two unused internal functions handling string lists. --- generator/states-connect-socket-activation.c | 59 ++++++------- generator/states-connect.c | 8 +- generator/states-newstyle-opt-meta-context.c | 22 ++--- lib/Makefile.am | 2 + lib/connect.c | 22 +---- lib/crypto.c | 36 ++++---- lib/handle.c | 28 +++--- lib/internal.h | 10 ++- lib/uri.c | 89 +++++++++----------- lib/utils.c | 43 +++------- 10 files changed, 133 insertions(+), 186 deletions(-) diff --git a/generator/states-connect-socket-activation.c b/generator/states-connect-socket-activation.c index ee08dff..5f4cfbd 100644 --- a/generator/states-connect-socket-activation.c +++ b/generator/states-connect-socket-activation.c @@ -47,11 +47,11 @@ extern char **environ; static char ** prepare_socket_activation_environment (void) { - char **env = NULL; + string_vector env = empty_vector; char *p0 = NULL, *p1 = NULL; - size_t i, len; - void *vp; + size_t i; + /* Reserve slots env[0] and env[1]. */ p0 = strdup ("LISTEN_PID=xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx"); if (p0 == NULL) goto err; @@ -59,38 +59,35 @@ prepare_socket_activation_environment (void) if (p1 == NULL) goto err; - /* Copy the current environment. */ - env = nbd_internal_copy_string_list (environ); - if (env == NULL) + if (string_vector_append (&env, p0) == -1 || + string_vector_append (&env, p1) == -1) goto err; - /* Reserve slots env[0] and env[1]. */ - len = nbd_internal_string_list_length (env); - vp = realloc (env, - sizeof (char *) * (len + 3)); /* include final NULL entry */ - if (vp == NULL) - goto err; - env = vp; - memmove (&env[2], &env[0], sizeof (char *) * (len + 1)); - - env[0] = p0; /* Code below assumes env[0] is LISTEN_PID. */ - env[1] = p1; - - /* Remove any existing LISTEN_PID or LISTEN_FDS instances. */ - for (i = 2; env[i] != NULL; ++i) { - if (strncmp (env[i], "LISTEN_PID=", PREFIX_LENGTH) == 0 || - strncmp (env[i], "LISTEN_FDS=", PREFIX_LENGTH) == 0) { - memmove (&env[i], &env[i+1], - sizeof (char *) * (nbd_internal_string_list_length (&env[i]))); - i--; + /* 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) { + char *copy = strdup (environ[i]); + if (copy == NULL) + goto err; + if (string_vector_append (&env, copy) == -1) { + free (copy); + goto err; + } } } - return env; + /* The environ must be NULL-terminated. */ + if (string_vector_append (&env, NULL) == -1) + goto err; + + return env.ptr; err: set_error (errno, "malloc"); - nbd_internal_free_string_list (env); + for (i = 2; env.ptr[i] != NULL; ++i) + free (env.ptr[i]); + free (env.ptr); free (p0); free (p1); return NULL; @@ -105,8 +102,8 @@ STATE_MACHINE { int flags; assert (!h->sock); - assert (h->argv); - assert (h->argv[0]); + assert (h->argv.ptr); + assert (h->argv.ptr[0]); /* Use /tmp instead of TMPDIR because we must ensure the path is * short enough to store in the sockaddr_un. On some platforms this @@ -201,8 +198,8 @@ STATE_MACHINE { signal (SIGPIPE, SIG_DFL); environ = env; - execvp (h->argv[0], h->argv); - nbd_internal_fork_safe_perror (h->argv[0]); + execvp (h->argv.ptr[0], h->argv.ptr); + nbd_internal_fork_safe_perror (h->argv.ptr[0]); if (errno == ENOENT) _exit (127); else diff --git a/generator/states-connect.c b/generator/states-connect.c index 33ae844..392879d 100644 --- a/generator/states-connect.c +++ b/generator/states-connect.c @@ -210,8 +210,8 @@ STATE_MACHINE { int flags; assert (!h->sock); - assert (h->argv); - assert (h->argv[0]); + assert (h->argv.ptr); + assert (h->argv.ptr[0]); if (socketpair (AF_UNIX, SOCK_STREAM|SOCK_CLOEXEC, 0, sv) == -1) { SET_NEXT_STATE (%.DEAD); set_error (errno, "socketpair"); @@ -237,8 +237,8 @@ STATE_MACHINE { /* Restore SIGPIPE back to SIG_DFL. */ signal (SIGPIPE, SIG_DFL); - execvp (h->argv[0], h->argv); - nbd_internal_fork_safe_perror (h->argv[0]); + execvp (h->argv.ptr[0], h->argv.ptr); + nbd_internal_fork_safe_perror (h->argv.ptr[0]); if (errno == ENOENT) _exit (127); else diff --git a/generator/states-newstyle-opt-meta-context.c b/generator/states-newstyle-opt-meta-context.c index fe82252..bbf155e 100644 --- a/generator/states-newstyle-opt-meta-context.c +++ b/generator/states-newstyle-opt-meta-context.c @@ -20,7 +20,7 @@ STATE_MACHINE { NEWSTYLE.OPT_META_CONTEXT.START: - size_t i, nr_queries; + size_t i; uint32_t len, opt; /* If the server doesn't support SRs then we must skip this group. @@ -38,8 +38,7 @@ STATE_MACHINE { else { assert (CALLBACK_IS_NULL (h->opt_cb.fn.context)); opt = NBD_OPT_SET_META_CONTEXT; - if (!h->structured_replies || - nbd_internal_string_list_length (h->request_meta_contexts) == 0) { + if (!h->structured_replies || h->request_meta_contexts.size == 0) { SET_NEXT_STATE (%^OPT_GO.START); return 0; } @@ -49,9 +48,8 @@ STATE_MACHINE { /* Calculate the length of the option request data. */ len = 4 /* exportname len */ + strlen (h->export_name) + 4 /* nr queries */; - nr_queries = nbd_internal_string_list_length (h->request_meta_contexts); - for (i = 0; i < nr_queries; ++i) - len += 4 /* length of query */ + strlen (h->request_meta_contexts[i]); + for (i = 0; i < h->request_meta_contexts.size; ++i) + len += 4 /* length of query */ + strlen (h->request_meta_contexts.ptr[i]); h->sbuf.option.version = htobe64 (NBD_NEW_VERSION); h->sbuf.option.option = htobe32 (opt); @@ -89,8 +87,7 @@ STATE_MACHINE { switch (send_from_wbuf (h)) { case -1: SET_NEXT_STATE (%.DEAD); return 0; case 0: - h->sbuf.nrqueries = - htobe32 (nbd_internal_string_list_length (h->request_meta_contexts)); + h->sbuf.nrqueries = htobe32 (h->request_meta_contexts.size); h->wbuf = &h->sbuf; h->wlen = sizeof h->sbuf.nrqueries; h->wflags = MSG_MORE; @@ -108,13 +105,12 @@ STATE_MACHINE { return 0; NEWSTYLE.OPT_META_CONTEXT.PREPARE_NEXT_QUERY: - const char *query = !h->request_meta_contexts ? NULL - : h->request_meta_contexts[h->querynum]; - - if (query == NULL) { /* end of list of requested meta contexts */ + if (h->querynum >= h->request_meta_contexts.size) { + /* end of list of requested meta contexts */ SET_NEXT_STATE (%PREPARE_FOR_REPLY); return 0; } + const char *query = h->request_meta_contexts.ptr[h->querynum]; h->sbuf.len = htobe32 (strlen (query)); h->wbuf = &h->sbuf.len; @@ -124,7 +120,7 @@ STATE_MACHINE { return 0; NEWSTYLE.OPT_META_CONTEXT.SEND_QUERYLEN: - const char *query = h->request_meta_contexts[h->querynum]; + const char *query = h->request_meta_contexts.ptr[h->querynum]; switch (send_from_wbuf (h)) { case -1: SET_NEXT_STATE (%.DEAD); return 0; diff --git a/lib/Makefile.am b/lib/Makefile.am index 9fd6331..968e41a 100644 --- a/lib/Makefile.am +++ b/lib/Makefile.am @@ -62,6 +62,7 @@ libnbd_la_SOURCES = \ libnbd_la_CPPFLAGS = \ -I$(top_srcdir)/include \ -I$(top_srcdir)/common/include \ + -I$(top_srcdir)/common/utils \ -Dsysconfdir=\"$(sysconfdir)\" \ $(NULL) libnbd_la_CFLAGS = \ @@ -71,6 +72,7 @@ libnbd_la_CFLAGS = \ $(LIBXML2_CFLAGS) \ $(NULL) libnbd_la_LIBADD = \ + $(top_builddir)/common/utils/libutils.la \ $(GNUTLS_LIBS) \ $(LIBXML2_LIBS) \ $(NULL) diff --git a/lib/connect.c b/lib/connect.c index 7e42b14..99781cd 100644 --- a/lib/connect.c +++ b/lib/connect.c @@ -249,18 +249,11 @@ nbd_unlocked_aio_connect_socket (struct nbd_handle *h, int sock) int nbd_unlocked_aio_connect_command (struct nbd_handle *h, char **argv) { - char **copy; - - copy = nbd_internal_copy_string_list (argv); - if (!copy) { - set_error (errno, "copy_string_list"); + if (nbd_internal_set_argv (&h->argv, argv) == -1) { + set_error (errno, "realloc"); return -1; } - if (h->argv) - nbd_internal_free_string_list (h->argv); - h->argv = copy; - return nbd_internal_run (h, cmd_connect_command); } @@ -268,17 +261,10 @@ int nbd_unlocked_aio_connect_systemd_socket_activation (struct nbd_handle *h, char **argv) { - char **copy; - - copy = nbd_internal_copy_string_list (argv); - if (!copy) { - set_error (errno, "copy_string_list"); + if (nbd_internal_set_argv (&h->argv, argv) == -1) { + set_error (errno, "realloc"); return -1; } - if (h->argv) - nbd_internal_free_string_list (h->argv); - h->argv = copy; - return nbd_internal_run (h, cmd_connect_sa); } diff --git a/lib/crypto.c b/lib/crypto.c index 516651c..a9b3789 100644 --- a/lib/crypto.c +++ b/lib/crypto.c @@ -31,6 +31,7 @@ #endif #include "internal.h" +#include "vector.h" int nbd_unlocked_set_tls (struct nbd_handle *h, int tls) @@ -107,11 +108,13 @@ nbd_unlocked_set_tls_username (struct nbd_handle *h, const char *username) return 0; } +DEFINE_VECTOR_TYPE (string, char) + char * nbd_unlocked_get_tls_username (struct nbd_handle *h) { char *s, *ret; - size_t len; + string str = empty_vector; if (h->tls_username) { ret = strdup (h->tls_username); @@ -137,31 +140,22 @@ nbd_unlocked_get_tls_username (struct nbd_handle *h) return ret; } - len = 16; - ret = NULL; - while (ret == NULL) { - char *oldret = ret; - - ret = realloc (oldret, len); - if (ret == NULL) { - set_error (errno, "realloc"); - free (oldret); - return NULL; + for (;;) { + if (getlogin_r (str.ptr, str.alloc) == 0) { + return str.ptr; } - - if (getlogin_r (ret, len) != 0) { - if (errno == ERANGE) { - /* Try again with a larger buffer. */ - len *= 2; - continue; - } + else if (errno != ERANGE) { set_error (errno, "getlogin_r"); - free (ret); + free (str.ptr); + return NULL; + } + /* Try again with a larger buffer. */ + if (string_reserve (&str, str.alloc == 0 ? 16 : str.alloc * 2) == -1) { + set_error (errno, "realloc"); + free (str.ptr); return NULL; } } - - return ret; } int diff --git a/lib/handle.c b/lib/handle.c index 35fe066..841514a 100644 --- a/lib/handle.c +++ b/lib/handle.c @@ -129,7 +129,8 @@ nbd_close (struct nbd_handle *h) free_cmd_list (h->cmds_to_issue); free_cmd_list (h->cmds_in_flight); free_cmd_list (h->cmds_done); - nbd_internal_free_string_list (h->argv); + string_vector_iter (&h->argv, (void *) free); + free (h->argv.ptr); if (h->sa_sockpath) { if (h->pid > 0) kill (h->pid, SIGTERM); @@ -153,7 +154,8 @@ nbd_close (struct nbd_handle *h) free (h->tls_certificates); free (h->tls_username); free (h->tls_psk_file); - nbd_internal_free_string_list (h->request_meta_contexts); + string_vector_iter (&h->request_meta_contexts, (void *) free); + free (h->request_meta_contexts.ptr); free (h->hname); pthread_mutex_destroy (&h->lock); free (h); @@ -285,8 +287,6 @@ int nbd_unlocked_add_meta_context (struct nbd_handle *h, const char *name) { char *copy; - size_t len; - char **list; if (strnlen (name, NBD_MAX_STRING + 1) > NBD_MAX_STRING) { set_error (ENAMETOOLONG, "meta context name too long for NBD protocol"); @@ -298,17 +298,12 @@ nbd_unlocked_add_meta_context (struct nbd_handle *h, const char *name) set_error (errno, "strdup"); return -1; } - len = nbd_internal_string_list_length (h->request_meta_contexts); - list = realloc (h->request_meta_contexts, - sizeof (char *) * (len+2 /* + new entry + NULL */)); - if (list == NULL) { + + if (string_vector_append (&h->request_meta_contexts, copy) == -1) { free (copy); set_error (errno, "realloc"); return -1; } - h->request_meta_contexts = list; - list[len] = copy; - list[len+1] = NULL; return 0; } @@ -316,21 +311,20 @@ nbd_unlocked_add_meta_context (struct nbd_handle *h, const char *name) ssize_t nbd_unlocked_get_nr_meta_contexts (struct nbd_handle *h) { - return nbd_internal_string_list_length (h->request_meta_contexts); + return h->request_meta_contexts.size; } char * nbd_unlocked_get_meta_context (struct nbd_handle *h, size_t i) { - size_t len = nbd_internal_string_list_length (h->request_meta_contexts); char *ret; - if (i >= len) { + if (i >= h->request_meta_contexts.size) { set_error (EINVAL, "meta context request out of range"); return NULL; } - ret = strdup (h->request_meta_contexts[i]); + ret = strdup (h->request_meta_contexts.ptr[i]); if (ret == NULL) set_error (errno, "strdup"); @@ -340,8 +334,8 @@ nbd_unlocked_get_meta_context (struct nbd_handle *h, size_t i) int nbd_unlocked_clear_meta_contexts (struct nbd_handle *h) { - nbd_internal_free_string_list (h->request_meta_contexts); - h->request_meta_contexts = NULL; + string_vector_iter (&h->request_meta_contexts, (void *) free); + string_vector_reset (&h->request_meta_contexts); return 0; } diff --git a/lib/internal.h b/lib/internal.h index ad1eeb9..d4f324e 100644 --- a/lib/internal.h +++ b/lib/internal.h @@ -40,6 +40,7 @@ #include "byte-swapping.h" #include "states.h" #include "unlocked.h" +#include "vector.h" /* Define unlikely macro, but only for GCC. These are used to move * debug and error handling code out of hot paths, making the hot path @@ -82,6 +83,8 @@ struct command_cb { nbd_completion_callback completion; }; +DEFINE_VECTOR_TYPE (string_vector, char *) + struct nbd_handle { /* Unique name assigned to this handle for debug messages * (to avoid having to print actual pointers). @@ -102,7 +105,7 @@ struct nbd_handle { /* Desired metadata contexts. */ bool request_sr; - char **request_meta_contexts; + string_vector request_meta_contexts; /* Allowed in URIs, see lib/uri.c. */ uint32_t uri_allow_transports; @@ -249,7 +252,7 @@ struct nbd_handle { * the subprocess so we can wait on it when the connection is * closed. */ - char **argv; + string_vector argv; pid_t pid; /* When using systemd socket activation, this directory and socket @@ -454,9 +457,8 @@ extern int nbd_internal_aio_get_direction (enum state state); /* utils.c */ extern void nbd_internal_hexdump (const void *data, size_t len, FILE *fp); -extern size_t nbd_internal_string_list_length (char **argv); -extern char **nbd_internal_copy_string_list (char **argv); extern void nbd_internal_free_string_list (char **argv); +extern int nbd_internal_set_argv (string_vector *v, char **argv); extern const char *nbd_internal_fork_safe_itoa (long v, char *buf, size_t len); extern void nbd_internal_fork_safe_perror (const char *s); extern char *nbd_internal_printable_buffer (const void *buf, size_t count); diff --git a/lib/uri.c b/lib/uri.c index 79e9b41..9f5a290 100644 --- a/lib/uri.c +++ b/lib/uri.c @@ -27,6 +27,7 @@ #include <assert.h> #include "internal.h" +#include "vector.h" #ifdef HAVE_LIBXML2 @@ -47,23 +48,24 @@ struct uri_query { char *value; }; +DEFINE_VECTOR_TYPE (uri_query_list, struct uri_query) + /* Parse the query_raw substring of a URI into a list of decoded queries. - * Return the length of the list, or -1 on error. + * Return 0 on success or -1 on error. */ static int -parse_uri_queries (const char *query_raw, struct uri_query **list) +parse_uri_queries (const char *query_raw, uri_query_list *list) { /* Borrows from libvirt's viruri.c:virURIParseParams() */ const char *end, *eq; const char *query = query_raw; - int nqueries = 0; - struct uri_query *tmp; + size_t i; if (!query || !*query) return 0; while (*query) { - char *name = NULL, *value = NULL; + struct uri_query q = {0}; /* Find the next separator, or end of the string. */ end = strchr (query, '&'); @@ -83,14 +85,14 @@ parse_uri_queries (const char *query_raw, struct uri_query **list) /* If there is no '=' character, then we have just "name" * and consistent with CGI.pm we assume value is "". */ - name = xmlURIUnescapeString (query, end - query, NULL); - if (!name) goto error; + q.name = xmlURIUnescapeString (query, end - query, NULL); + if (!q.name) goto error; } else if (eq+1 == end) { /* Or if we have "name=" here (works around annoying * problem when calling xmlURIUnescapeString with len = 0). */ - name = xmlURIUnescapeString (query, eq - query, NULL); - if (!name) goto error; + q.name = xmlURIUnescapeString (query, eq - query, NULL); + if (!q.name) goto error; } else if (query == eq) { /* If the '=' character is at the beginning then we have * "=value" and consistent with CGI.pm we _ignore_ this. @@ -98,50 +100,43 @@ parse_uri_queries (const char *query_raw, struct uri_query **list) goto next; } else { /* Otherwise it's "name=value". */ - name = xmlURIUnescapeString (query, eq - query, NULL); - if (!name) + q.name = xmlURIUnescapeString (query, eq - query, NULL); + if (!q.name) goto error; - value = xmlURIUnescapeString (eq+1, end - (eq+1), NULL); - if (!value) { - free (name); + q.value = xmlURIUnescapeString (eq+1, end - (eq+1), NULL); + if (!q.value) { + free (q.name); goto error; } } - if (!value) { - value = strdup (""); - if (!value) { - free (name); + if (!q.value) { + q.value = strdup (""); + if (!q.value) { + free (q.name); goto error; } } - /* Append to the parameter set. */ - tmp = realloc (*list, sizeof (*tmp) * (nqueries + 1)); - if (tmp == NULL) { - free (name); - free (value); + /* Append to the list of queries. */ + if (uri_query_list_append (list, q) == -1) { + free (q.name); + free (q.value); goto error; } - *list = tmp; - tmp[nqueries].name = name; - tmp[nqueries].value = value; - nqueries++; next: query = end; if (*query) query++; /* skip '&' separator */ } - return nqueries; + return 0; error: - tmp = *list; - while (nqueries-- > 0) { - free (tmp[nqueries].name); - free (tmp[nqueries].value); + for (i = 0; i < list->size; ++list) { + free (list->ptr[i].name); + free (list->ptr[i].value); } - free (tmp); - *list = NULL; + uri_query_list_reset (list); return -1; } @@ -151,8 +146,7 @@ nbd_unlocked_aio_connect_uri (struct nbd_handle *h, const char *raw_uri) xmlURIPtr uri = NULL; enum { tcp, unix_sock, vsock } transport; bool tls, socket_required; - struct uri_query *queries = NULL; - int nqueries = -1; + uri_query_list queries = empty_vector; int i, r; int ret = -1; const char *unixsocket = NULL; @@ -164,8 +158,7 @@ nbd_unlocked_aio_connect_uri (struct nbd_handle *h, const char *raw_uri) set_error (EINVAL, "unable to parse URI: %s", raw_uri); goto cleanup; } - nqueries = parse_uri_queries (uri->query_raw, &queries); - if (nqueries == -1) { + if (parse_uri_queries (uri->query_raw, &queries) == -1) { set_error (EINVAL, "unable to parse URI queries: %s", uri->query_raw); goto cleanup; } @@ -235,9 +228,9 @@ nbd_unlocked_aio_connect_uri (struct nbd_handle *h, const char *raw_uri) } /* Parse the socket parameter. */ - for (i = 0; i < nqueries; i++) { - if (strcmp (queries[i].name, "socket") == 0) - unixsocket = queries[i].value; + for (i = 0; i < queries.size; i++) { + if (strcmp (queries.ptr[i].name, "socket") == 0) + unixsocket = queries.ptr[i].value; } if (socket_required && !unixsocket) { @@ -257,15 +250,15 @@ nbd_unlocked_aio_connect_uri (struct nbd_handle *h, const char *raw_uri) goto cleanup; /* Look for some tls-* parameters. XXX More to come. */ - for (i = 0; i < nqueries; i++) { - if (strcmp (queries[i].name, "tls-psk-file") == 0) { + for (i = 0; i < queries.size; i++) { + if (strcmp (queries.ptr[i].name, "tls-psk-file") == 0) { if (! h->uri_allow_local_file) { set_error (EPERM, "local file access (tls-psk-file) is not allowed, " "call nbd_set_uri_allow_local_file to enable this"); goto cleanup; } - if (nbd_unlocked_set_tls_psk_file (h, queries[i].value) == -1) + if (nbd_unlocked_set_tls_psk_file (h, queries.ptr[i].value) == -1) goto cleanup; } } @@ -333,11 +326,11 @@ nbd_unlocked_aio_connect_uri (struct nbd_handle *h, const char *raw_uri) ret = 0; cleanup: - while (nqueries-- > 0) { - free (queries[nqueries].name); - free (queries[nqueries].value); + for (i = 0; i < queries.size; ++i) { + free (queries.ptr[i].name); + free (queries.ptr[i].value); } - free (queries); + free (queries.ptr); xmlFreeURI (uri); return ret; } diff --git a/lib/utils.c b/lib/utils.c index 6e5bdeb..c3f4c4c 100644 --- a/lib/utils.c +++ b/lib/utils.c @@ -52,42 +52,25 @@ nbd_internal_hexdump (const void *data, size_t len, FILE *fp) } } -size_t -nbd_internal_string_list_length (char **argv) +/* Replace the string_vector with a deep copy of argv (including final NULL). */ +int +nbd_internal_set_argv (string_vector *v, char **argv) { - size_t ret; + size_t i; - if (argv == NULL) - return 0; + string_vector_reset (v); - for (ret = 0; argv[ret] != NULL; ++ret) - ; - return ret; -} - -char ** -nbd_internal_copy_string_list (char **argv) -{ - size_t i, j, n; - char **ret; - - n = nbd_internal_string_list_length (argv); - ret = calloc (n+1, sizeof (char *)); - if (!ret) - return NULL; - - for (i = 0; i < n; ++i) { - ret[i] = strdup (argv[i]); - if (ret[i] == NULL) { - for (j = 0; j < i; ++j) - free (ret[j]); - free (ret); - return NULL; + for (i = 0; argv[i] != NULL; ++i) { + char *copy = strdup (argv[i]); + if (copy == NULL) + return -1; + if (string_vector_append (v, copy) == -1) { + free (copy); + return -1; } } - ret[n] = NULL; - return ret; + return string_vector_append (v, NULL); } void -- 2.29.0.rc2