[Libguestfs] [nbdkit PATCH v2 3/7] server: Switch to fixed-length conn->exportname

[Eric Blake]

Since each connection already has a malloc()d struct conn, and the NBD
protocol places a fixed length on the maximum string at 4k, it's not
that much harder to just directly set aside 4k in the connection
struct instead of allocating a name on the fly.  In the common case of
short names, it does mean that a connection now occupies slightly more
unused heap space, but these days, 4k overhead is probably in the
noise.  Doing this makes it easier for an upcoming patch to refactor
name validation without having to worry about an allocation failure,
which in turn makes it easier to keep the connection alive even after
failing NBD_OPT_GO (where an allocation failure would have to be
handled differently than the diagnosis of an invalid client request).

This also points out a spot where we need to enlarge
MAX_OPTION_LENGTH, but that will be a separate patch.

Signed-off-by: Eric Blake <eblake at redhat.com>
---
 server/internal.h                    |  3 ++-
 server/Makefile.am                   |  1 +
 server/connections.c                 |  7 -------
 server/protocol-handshake-newstyle.c | 17 +++++------------
 4 files changed, 8 insertions(+), 20 deletions(-)

diff --git a/server/internal.h b/server/internal.h
index fbabce69..e6a22f1a 100644
--- a/server/internal.h
+++ b/server/internal.h
@@ -43,6 +43,7 @@
 #include "nbdkit-plugin.h"
 #include "nbdkit-filter.h"
 #include "cleanup.h"
+#include "nbd-protocol.h"

 #ifdef __APPLE__
 #define UNIX_PATH_MAX 104
@@ -198,7 +199,7 @@ struct connection {
   struct b_conn_handle *handles;
   size_t nr_handles;

-  char *exportname;
+  char exportname[NBD_MAX_STRING + 1];
   uint32_t cflags;
   uint16_t eflags;
   bool using_tls;
diff --git a/server/Makefile.am b/server/Makefile.am
index a0417639..c846fc7a 100644
--- a/server/Makefile.am
+++ b/server/Makefile.am
@@ -134,6 +134,7 @@ test_public_SOURCES = \
 test_public_CPPFLAGS = \
 	-I$(top_srcdir)/include \
 	-I$(top_srcdir)/common/include \
+	-I$(top_srcdir)/common/protocol \
 	-I$(top_srcdir)/common/utils \
 	$(NULL)
 test_public_CFLAGS = $(WARNINGS_CFLAGS) $(VALGRIND_CFLAGS)
diff --git a/server/connections.c b/server/connections.c
index eeda85d5..27cf202b 100644
--- a/server/connections.c
+++ b/server/connections.c
@@ -258,11 +258,6 @@ new_connection (int sockin, int sockout, int nworkers)

   conn->status_pipe[0] = conn->status_pipe[1] = -1;

-  conn->exportname = strdup ("");
-  if (conn->exportname == NULL) {
-    perror ("strdup");
-    goto error;
-  }
   conn->handles = calloc (backend->i + 1, sizeof *conn->handles);
   if (conn->handles == NULL) {
     perror ("malloc");
@@ -335,7 +330,6 @@ new_connection (int sockin, int sockout, int nworkers)
     close (conn->status_pipe[0]);
   if (conn->status_pipe[1] >= 0)
     close (conn->status_pipe[1]);
-  free (conn->exportname);
   free (conn->handles);
   free (conn);
   return NULL;
@@ -383,7 +377,6 @@ free_connection (struct connection *conn)
   pthread_mutex_destroy (&conn->status_lock);

   free (conn->handles);
-  free (conn->exportname);
   free (conn);
 }

diff --git a/server/protocol-handshake-newstyle.c b/server/protocol-handshake-newstyle.c
index d0fb4dd7..0cbcc3d5 100644
--- a/server/protocol-handshake-newstyle.c
+++ b/server/protocol-handshake-newstyle.c
@@ -277,12 +277,7 @@ negotiate_handshake_newstyle_options (struct connection *conn)
       data[optlen] = '\0';
       debug ("newstyle negotiation: %s: client requested export '%s'",
              name_of_nbd_opt (option), data);
-      free (conn->exportname);
-      conn->exportname = malloc (optlen+1);
-      if (conn->exportname == NULL) {
-        nbdkit_error ("malloc: %m");
-        return -1;
-      }
+      assert (optlen < sizeof conn->exportname);
       strcpy (conn->exportname, data);

       /* We have to finish the handshake by sending handshake_finish. */
@@ -420,12 +415,10 @@ negotiate_handshake_newstyle_options (struct connection *conn)
         /* As with NBD_OPT_EXPORT_NAME we print the export name and
          * save it in the connection.
          */
-        free (conn->exportname);
-        conn->exportname = malloc (exportnamelen+1);
-        if (conn->exportname == NULL) {
-          nbdkit_error ("malloc: %m");
-          return -1;
-        }
+        /* FIXME: Our current MAX_OPTION_LENGTH prevents us from receiving
+         * an export name at the full NBD_MAX_STRING length.
+         */
+        assert (exportnamelen < sizeof conn->exportname);
         memcpy (conn->exportname, &data[4], exportnamelen);
         conn->exportname[exportnamelen] = '\0';
         debug ("newstyle negotiation: %s: client requested export '%s'",
-- 
2.21.0