Richard W.M. Jones
2020-Feb-12 13:40 UTC
[Libguestfs] [PATCH nbdkit 1/3] server: Rename global backend pointer to "top".
It's confusing to use the same terminology for a single backend as for the linked list of backends. In particular it's often not clear if we're calling the next backend or the whole chain of backends. --- server/internal.h | 14 ++++++++++-- server/connections.c | 20 ++++++++--------- server/locks.c | 2 +- server/main.c | 32 ++++++++++++++-------------- server/plugins.c | 2 +- server/protocol-handshake-newstyle.c | 8 +++---- server/protocol-handshake.c | 28 ++++++++++++------------ server/protocol.c | 18 ++++++++-------- 8 files changed, 67 insertions(+), 57 deletions(-) diff --git a/server/internal.h b/server/internal.h index 45ac60ad..c3622671 100644 --- a/server/internal.h +++ b/server/internal.h @@ -118,8 +118,18 @@ extern char *unixsocket; extern const char *user, *group; extern bool verbose; -extern struct backend *backend; -#define for_each_backend(b) for (b = backend; b != NULL; b = b->next) +/* Linked list of backends. Each backend struct is followed by either + * a filter or plugin struct. "top" points to the first one. They + * are linked through the backend->next field. + * + * ┌──────────┐ ┌──────────┐ ┌──────────┐ + * top ───▶│ backend │───▶│ backend │───▶│ backend │ + * │ b->i = 2 │ │ b->i = 1 │ │ b->i = 0 │ + * │ filter │ │ filter │ │ plugin │ + * └──────────┘ └──────────┘ └──────────┘ + */ +extern struct backend *top; +#define for_each_backend(b) for (b = top; b != NULL; b = b->next) /* quit.c */ extern volatile int quit; diff --git a/server/connections.c b/server/connections.c index a2049325..7e9584b3 100644 --- a/server/connections.c +++ b/server/connections.c @@ -146,23 +146,23 @@ handle_single_connection (int sockin, int sockout) lock_connection (); - if (backend->thread_model (backend) < NBDKIT_THREAD_MODEL_PARALLEL || + if (top->thread_model (top) < NBDKIT_THREAD_MODEL_PARALLEL || nworkers == 1) nworkers = 0; conn = new_connection (sockin, sockout, nworkers); if (!conn) goto done; - /* NB: because of an asynchronous exit backend can be set to NULL at + /* NB: because of an asynchronous exit top can be set to NULL at * just about any time. */ - if (backend) - plugin_name = backend->plugin_name (backend); + if (top) + plugin_name = top->plugin_name (top); else plugin_name = "(unknown)"; threadlocal_set_name (plugin_name); - if (backend && backend->preconnect (backend, read_only) == -1) + if (top && top->preconnect (top, read_only) == -1) goto done; /* NBD handshake. @@ -225,7 +225,7 @@ handle_single_connection (int sockin, int sockout) /* Finalize (for filters), called just before close. */ lock_request (); - r = backend_finalize (backend); + r = backend_finalize (top); unlock_request (); if (r == -1) goto done; @@ -251,12 +251,12 @@ new_connection (int sockin, int sockout, int nworkers) conn->status_pipe[0] = conn->status_pipe[1] = -1; - conn->handles = calloc (backend->i + 1, sizeof *conn->handles); + conn->handles = calloc (top->i + 1, sizeof *conn->handles); if (conn->handles == NULL) { perror ("malloc"); goto error; } - conn->nr_handles = backend->i + 1; + conn->nr_handles = top->i + 1; for_each_backend (b) reset_b_conn_handle (&conn->handles[b->i]); @@ -277,7 +277,7 @@ new_connection (int sockin, int sockout, int nworkers) * we aren't accepting until the plugin is not running, making * non-atomicity okay. */ - assert (backend->thread_model (backend) <+ assert (top->thread_model (top) < NBDKIT_THREAD_MODEL_SERIALIZE_ALL_REQUESTS); lock_request (NULL); if (pipe (conn->status_pipe)) { @@ -354,7 +354,7 @@ free_connection (struct connection *conn) */ if (!quit) { lock_request (); - backend_close (backend); + backend_close (top); unlock_request (); } diff --git a/server/locks.c b/server/locks.c index f005710d..6211648d 100644 --- a/server/locks.c +++ b/server/locks.c @@ -69,7 +69,7 @@ name_of_thread_model (int model) void lock_init_thread_model (void) { - thread_model = backend->thread_model (backend); + thread_model = top->thread_model (top); debug ("using thread model: %s", name_of_thread_model (thread_model)); } diff --git a/server/main.c b/server/main.c index 550a8714..dbeca624 100644 --- a/server/main.c +++ b/server/main.c @@ -102,8 +102,8 @@ bool verbose; /* -v */ bool vsock; /* --vsock */ unsigned int socket_activation /* $LISTEN_FDS and $LISTEN_PID set */; -/* The currently loaded plugin. */ -struct backend *backend; +/* The linked list of zero or more filters, and one plugin. */ +struct backend *top; static char *random_fifo_dir = NULL; static char *random_fifo = NULL; @@ -555,10 +555,10 @@ main (int argc, char *argv[]) /* Open the plugin (first) and then wrap the plugin with the * filters. The filters are wrapped in reverse order that they - * appear on the command line so that in the end ‘backend’ points to + * appear on the command line so that in the end ‘top’ points to * the first filter on the command line. */ - backend = open_plugin_so (0, filename, short_name); + top = open_plugin_so (0, filename, short_name); i = 1; while (filter_filenames) { struct filter_filename *t = filter_filenames; @@ -566,7 +566,7 @@ main (int argc, char *argv[]) filename = t->filename; short_name = is_short_name (filename); - backend = open_filter_so (backend, i++, filename, short_name); + top = open_filter_so (top, i++, filename, short_name); filter_filenames = t->next; free (t); @@ -586,7 +586,7 @@ main (int argc, char *argv[]) printf ("\n"); b->usage (b); } - backend->free (backend); + top->free (top); exit (EXIT_SUCCESS); } @@ -601,7 +601,7 @@ main (int argc, char *argv[]) printf (" %s", v); printf ("\n"); } - backend->free (backend); + top->free (top); exit (EXIT_SUCCESS); } @@ -615,16 +615,16 @@ main (int argc, char *argv[]) * first parameter is bare it is prefixed with the key "script", and * any other bare parameters are errors. */ - magic_config_key = backend->magic_config_key (backend); + magic_config_key = top->magic_config_key (top); for (i = 0; optind < argc; ++i, ++optind) { p = strchr (argv[optind], '='); if (p && is_config_key (argv[optind], p - argv[optind])) { /* key=value */ *p = '\0'; - backend->config (backend, argv[optind], p+1); + top->config (top, argv[optind], p+1); } else if (magic_config_key == NULL) { if (i == 0) /* magic script parameter */ - backend->config (backend, "script", argv[optind]); + top->config (top, "script", argv[optind]); else { fprintf (stderr, "%s: expecting key=value on the command line but got: %s\n", @@ -633,7 +633,7 @@ main (int argc, char *argv[]) } } else { /* magic config key */ - backend->config (backend, magic_config_key, argv[optind]); + top->config (top, magic_config_key, argv[optind]); } } @@ -643,12 +643,12 @@ main (int argc, char *argv[]) * parameters. */ if (dump_plugin) { - backend->dump_fields (backend); - backend->free (backend); + top->dump_fields (top); + top->free (top); exit (EXIT_SUCCESS); } - backend->config_complete (backend); + top->config_complete (top); /* Select the correct thread model based on config. */ lock_init_thread_model (); @@ -660,8 +660,8 @@ main (int argc, char *argv[]) start_serving (); - backend->free (backend); - backend = NULL; + top->free (top); + top = NULL; free (unixsocket); free (pidfile); diff --git a/server/plugins.c b/server/plugins.c index 9595269c..16b4099b 100644 --- a/server/plugins.c +++ b/server/plugins.c @@ -144,7 +144,7 @@ plugin_dump_fields (struct backend *b) printf ("max_thread_model=%s\n", name_of_thread_model (p->plugin._thread_model)); printf ("thread_model=%s\n", - name_of_thread_model (backend->thread_model (backend))); + name_of_thread_model (top->thread_model (top))); printf ("errno_is_preserved=%d\n", !!p->plugin.errno_is_preserved); if (p->plugin.magic_config_key) printf ("magic_config_key=%s\n", p->plugin.magic_config_key); diff --git a/server/protocol-handshake-newstyle.c b/server/protocol-handshake-newstyle.c index 41b2a6e4..946060ac 100644 --- a/server/protocol-handshake-newstyle.c +++ b/server/protocol-handshake-newstyle.c @@ -478,9 +478,9 @@ negotiate_handshake_newstyle_options (void) * disconnecting. */ if (finish_newstyle_options (&exportsize) == -1) { - if (backend_finalize (backend) == -1) + if (backend_finalize (top) == -1) return -1; - backend_close (backend); + backend_close (top); if (send_newstyle_option_reply (option, NBD_REP_ERR_UNKNOWN) == -1) return -1; continue; @@ -518,9 +518,9 @@ negotiate_handshake_newstyle_options (void) return -1; if (option == NBD_OPT_INFO) { - if (backend_finalize (backend) == -1) + if (backend_finalize (top) == -1) return -1; - backend_close (backend); + backend_close (top); } break; diff --git a/server/protocol-handshake.c b/server/protocol-handshake.c index a32fcde0..70ea4933 100644 --- a/server/protocol-handshake.c +++ b/server/protocol-handshake.c @@ -79,14 +79,14 @@ protocol_common_open (uint64_t *exportsize, uint16_t *flags) uint16_t eflags = NBD_FLAG_HAS_FLAGS; int fl; - if (backend_open (backend, read_only) == -1) + if (backend_open (top, read_only) == -1) return -1; /* Prepare (for filters), called just after open. */ - if (backend_prepare (backend) == -1) + if (backend_prepare (top) == -1) return -1; - size = backend_get_size (backend); + size = backend_get_size (top); if (size == -1) return -1; if (size < 0) { @@ -98,57 +98,57 @@ protocol_common_open (uint64_t *exportsize, uint16_t *flags) /* Check all flags even if they won't be advertised, to prime the * cache and make later request validation easier. */ - fl = backend_can_write (backend); + fl = backend_can_write (top); if (fl == -1) return -1; if (!fl) eflags |= NBD_FLAG_READ_ONLY; - fl = backend_can_zero (backend); + fl = backend_can_zero (top); if (fl == -1) return -1; if (fl) eflags |= NBD_FLAG_SEND_WRITE_ZEROES; - fl = backend_can_fast_zero (backend); + fl = backend_can_fast_zero (top); if (fl == -1) return -1; if (fl) eflags |= NBD_FLAG_SEND_FAST_ZERO; - fl = backend_can_trim (backend); + fl = backend_can_trim (top); if (fl == -1) return -1; if (fl) eflags |= NBD_FLAG_SEND_TRIM; - fl = backend_can_fua (backend); + fl = backend_can_fua (top); if (fl == -1) return -1; if (fl) eflags |= NBD_FLAG_SEND_FUA; - fl = backend_can_flush (backend); + fl = backend_can_flush (top); if (fl == -1) return -1; if (fl) eflags |= NBD_FLAG_SEND_FLUSH; - fl = backend_is_rotational (backend); + fl = backend_is_rotational (top); if (fl == -1) return -1; if (fl) eflags |= NBD_FLAG_ROTATIONAL; /* multi-conn is useless if parallel connections are not allowed. */ - fl = backend_can_multi_conn (backend); + fl = backend_can_multi_conn (top); if (fl == -1) return -1; - if (fl && (backend->thread_model (backend) > + if (fl && (top->thread_model (top) > NBDKIT_THREAD_MODEL_SERIALIZE_CONNECTIONS)) eflags |= NBD_FLAG_CAN_MULTI_CONN; - fl = backend_can_cache (backend); + fl = backend_can_cache (top); if (fl == -1) return -1; if (fl) @@ -159,7 +159,7 @@ protocol_common_open (uint64_t *exportsize, uint16_t *flags) * not have to worry about errors, and makes test-layers easier to * write. */ - fl = backend_can_extents (backend); + fl = backend_can_extents (top); if (fl == -1) return -1; diff --git a/server/protocol.c b/server/protocol.c index d41ad569..b56d16bd 100644 --- a/server/protocol.c +++ b/server/protocol.c @@ -72,7 +72,7 @@ validate_request (uint16_t cmd, uint16_t flags, uint64_t offset, uint32_t count, case NBD_CMD_TRIM: case NBD_CMD_WRITE_ZEROES: case NBD_CMD_BLOCK_STATUS: - if (!backend_valid_range (backend, offset, count)) { + if (!backend_valid_range (top, offset, count)) { /* XXX Allow writes to extend the disk? */ nbdkit_error ("invalid request: %s: offset and count are out of range: " "offset=%" PRIu64 " count=%" PRIu32, @@ -238,31 +238,31 @@ handle_request (uint16_t cmd, uint16_t flags, uint64_t offset, uint32_t count, switch (cmd) { case NBD_CMD_READ: - if (backend_pread (backend, buf, count, offset, 0, &err) == -1) + if (backend_pread (top, buf, count, offset, 0, &err) == -1) return err; break; case NBD_CMD_WRITE: if (flags & NBD_CMD_FLAG_FUA) f |= NBDKIT_FLAG_FUA; - if (backend_pwrite (backend, buf, count, offset, f, &err) == -1) + if (backend_pwrite (top, buf, count, offset, f, &err) == -1) return err; break; case NBD_CMD_FLUSH: - if (backend_flush (backend, 0, &err) == -1) + if (backend_flush (top, 0, &err) == -1) return err; break; case NBD_CMD_TRIM: if (flags & NBD_CMD_FLAG_FUA) f |= NBDKIT_FLAG_FUA; - if (backend_trim (backend, count, offset, f, &err) == -1) + if (backend_trim (top, count, offset, f, &err) == -1) return err; break; case NBD_CMD_CACHE: - if (backend_cache (backend, count, offset, 0, &err) == -1) + if (backend_cache (top, count, offset, 0, &err) == -1) return err; break; @@ -273,14 +273,14 @@ handle_request (uint16_t cmd, uint16_t flags, uint64_t offset, uint32_t count, f |= NBDKIT_FLAG_FUA; if (flags & NBD_CMD_FLAG_FAST_ZERO) f |= NBDKIT_FLAG_FAST_ZERO; - if (backend_zero (backend, count, offset, f, &err) == -1) + if (backend_zero (top, count, offset, f, &err) == -1) return err; break; case NBD_CMD_BLOCK_STATUS: if (flags & NBD_CMD_FLAG_REQ_ONE) f |= NBDKIT_FLAG_REQ_ONE; - if (backend_extents (backend, count, offset, f, + if (backend_extents (top, count, offset, f, extents, &err) == -1) return err; break; @@ -683,7 +683,7 @@ protocol_recv_request_send_reply (void) /* Allocate the extents list for block status only. */ if (cmd == NBD_CMD_BLOCK_STATUS) { - extents = nbdkit_extents_new (offset, backend_get_size (backend)); + extents = nbdkit_extents_new (offset, backend_get_size (top)); if (extents == NULL) { error = ENOMEM; goto send_reply; -- 2.25.0
Richard W.M. Jones
2020-Feb-12 13:40 UTC
[Libguestfs] [PATCH nbdkit 2/3] server: Rename ‘struct b_conn_handle’ to plain ‘struct handle’.
Also add an inline accessor function (get_handle). This also removes the unused function connection_get_handle. --- server/internal.h | 24 ++++++++++++++++----- server/backend.c | 50 ++++++++++++++++++++++---------------------- server/connections.c | 11 +--------- 3 files changed, 45 insertions(+), 40 deletions(-) diff --git a/server/internal.h b/server/internal.h index c3622671..9d314bf8 100644 --- a/server/internal.h +++ b/server/internal.h @@ -170,16 +170,24 @@ typedef int (*connection_send_function) (const void *buf, size_t len, __attribute__((__nonnull__ (1))); typedef void (*connection_close_function) (void); +/* struct handle stores data per connection and backend. Primarily + * this is the filter or plugin handle, but other state is also stored + * here. + * + * Use get_handle (conn, 0) to return the struct handle for the + * plugin, and get_handle (conn, b->i) to return the struct handle for + * the i'th backend (if b->i >= 1 then for a filter). + */ enum { HANDLE_OPEN = 1, /* Set if .open passed, so .close is needed */ HANDLE_CONNECTED = 2, /* Set if .prepare passed, so .finalize is needed */ HANDLE_FAILED = 4, /* Set if .finalize failed */ }; -struct b_conn_handle { - void *handle; +struct handle { + void *handle; /* Plugin or filter handle. */ - unsigned char state; /* Bitmask of HANDLE_* values */ + unsigned char state; /* Bitmask of HANDLE_* values */ uint64_t exportsize; int can_write; @@ -195,7 +203,7 @@ struct b_conn_handle { }; static inline void -reset_b_conn_handle (struct b_conn_handle *h) +reset_handle (struct handle *h) { h->handle = NULL; h->state = 0; @@ -222,7 +230,7 @@ struct connection { void *crypto_session; int nworkers; - struct b_conn_handle *handles; + struct handle *handles; /* One per plugin and filter. */ size_t nr_handles; char exportname[NBD_MAX_STRING + 1]; @@ -239,6 +247,12 @@ struct connection { connection_close_function close; }; +static inline struct handle * +get_handle (struct connection *conn, int i) +{ + return &conn->handles[i]; +} + extern void handle_single_connection (int sockin, int sockout); extern int connection_get_status (void); extern int connection_set_status (int value); diff --git a/server/backend.c b/server/backend.c index 616c24d8..9669ada1 100644 --- a/server/backend.c +++ b/server/backend.c @@ -154,7 +154,7 @@ int backend_open (struct backend *b, int readonly) { GET_CONN; - struct b_conn_handle *h = &conn->handles[b->i]; + struct handle *h = get_handle (conn, b->i); controlpath_debug ("%s: open readonly=%d", b->name, readonly); @@ -178,7 +178,7 @@ backend_open (struct backend *b, int readonly) h->state |= HANDLE_OPEN; if (b->i) /* A filter must not succeed unless its backend did also */ - assert (conn->handles[b->i - 1].handle); + assert (get_handle (conn, b->i-1)->handle != NULL); return 0; } @@ -186,7 +186,7 @@ int backend_prepare (struct backend *b) { GET_CONN; - struct b_conn_handle *h = &conn->handles[b->i]; + struct handle *h = get_handle (conn, b->i); assert (h->handle); assert ((h->state & (HANDLE_OPEN | HANDLE_CONNECTED)) == HANDLE_OPEN); @@ -209,7 +209,7 @@ int backend_finalize (struct backend *b) { GET_CONN; - struct b_conn_handle *h = &conn->handles[b->i]; + struct handle *h = get_handle (conn, b->i); /* Call these in reverse order to .prepare above, starting from the * filter furthest away from the plugin, and matching .close order. @@ -238,7 +238,7 @@ void backend_close (struct backend *b) { GET_CONN; - struct b_conn_handle *h = &conn->handles[b->i]; + struct handle *h = get_handle (conn, b->i); /* outer-to-inner order, opposite .open */ controlpath_debug ("%s: close", b->name); @@ -249,7 +249,7 @@ backend_close (struct backend *b) } else assert (! (h->state & HANDLE_OPEN)); - reset_b_conn_handle (h); + reset_handle (h); if (b->i) backend_close (b->next); } @@ -258,7 +258,7 @@ bool backend_valid_range (struct backend *b, uint64_t offset, uint32_t count) { GET_CONN; - struct b_conn_handle *h = &conn->handles[b->i]; + struct handle *h = get_handle (conn, b->i); assert (h->exportsize <= INT64_MAX); /* Guaranteed by negotiation phase */ return count > 0 && offset <= h->exportsize && @@ -291,7 +291,7 @@ int64_t backend_get_size (struct backend *b) { GET_CONN; - struct b_conn_handle *h = &conn->handles[b->i]; + struct handle *h = get_handle (conn, b->i); controlpath_debug ("%s: get_size", b->name); @@ -305,7 +305,7 @@ int backend_can_write (struct backend *b) { GET_CONN; - struct b_conn_handle *h = &conn->handles[b->i]; + struct handle *h = get_handle (conn, b->i); controlpath_debug ("%s: can_write", b->name); @@ -319,7 +319,7 @@ int backend_can_flush (struct backend *b) { GET_CONN; - struct b_conn_handle *h = &conn->handles[b->i]; + struct handle *h = get_handle (conn, b->i); controlpath_debug ("%s: can_flush", b->name); @@ -333,7 +333,7 @@ int backend_is_rotational (struct backend *b) { GET_CONN; - struct b_conn_handle *h = &conn->handles[b->i]; + struct handle *h = get_handle (conn, b->i); controlpath_debug ("%s: is_rotational", b->name); @@ -347,7 +347,7 @@ int backend_can_trim (struct backend *b) { GET_CONN; - struct b_conn_handle *h = &conn->handles[b->i]; + struct handle *h = get_handle (conn, b->i); int r; controlpath_debug ("%s: can_trim", b->name); @@ -368,7 +368,7 @@ int backend_can_zero (struct backend *b) { GET_CONN; - struct b_conn_handle *h = &conn->handles[b->i]; + struct handle *h = get_handle (conn, b->i); int r; controlpath_debug ("%s: can_zero", b->name); @@ -389,7 +389,7 @@ int backend_can_fast_zero (struct backend *b) { GET_CONN; - struct b_conn_handle *h = &conn->handles[b->i]; + struct handle *h = get_handle (conn, b->i); int r; controlpath_debug ("%s: can_fast_zero", b->name); @@ -410,7 +410,7 @@ int backend_can_extents (struct backend *b) { GET_CONN; - struct b_conn_handle *h = &conn->handles[b->i]; + struct handle *h = get_handle (conn, b->i); controlpath_debug ("%s: can_extents", b->name); @@ -424,7 +424,7 @@ int backend_can_fua (struct backend *b) { GET_CONN; - struct b_conn_handle *h = &conn->handles[b->i]; + struct handle *h = get_handle (conn, b->i); int r; controlpath_debug ("%s: can_fua", b->name); @@ -445,7 +445,7 @@ int backend_can_multi_conn (struct backend *b) { GET_CONN; - struct b_conn_handle *h = &conn->handles[b->i]; + struct handle *h = get_handle (conn, b->i); assert (h->handle && (h->state & HANDLE_CONNECTED)); controlpath_debug ("%s: can_multi_conn", b->name); @@ -459,7 +459,7 @@ int backend_can_cache (struct backend *b) { GET_CONN; - struct b_conn_handle *h = &conn->handles[b->i]; + struct handle *h = get_handle (conn, b->i); controlpath_debug ("%s: can_cache", b->name); @@ -475,7 +475,7 @@ backend_pread (struct backend *b, uint32_t flags, int *err) { GET_CONN; - struct b_conn_handle *h = &conn->handles[b->i]; + struct handle *h = get_handle (conn, b->i); int r; assert (h->handle && (h->state & HANDLE_CONNECTED)); @@ -496,7 +496,7 @@ backend_pwrite (struct backend *b, uint32_t flags, int *err) { GET_CONN; - struct b_conn_handle *h = &conn->handles[b->i]; + struct handle *h = get_handle (conn, b->i); bool fua = !!(flags & NBDKIT_FLAG_FUA); int r; @@ -520,7 +520,7 @@ backend_flush (struct backend *b, uint32_t flags, int *err) { GET_CONN; - struct b_conn_handle *h = &conn->handles[b->i]; + struct handle *h = get_handle (conn, b->i); int r; assert (h->handle && (h->state & HANDLE_CONNECTED)); @@ -540,7 +540,7 @@ backend_trim (struct backend *b, int *err) { GET_CONN; - struct b_conn_handle *h = &conn->handles[b->i]; + struct handle *h = get_handle (conn, b->i); bool fua = !!(flags & NBDKIT_FLAG_FUA); int r; @@ -566,7 +566,7 @@ backend_zero (struct backend *b, int *err) { GET_CONN; - struct b_conn_handle *h = &conn->handles[b->i]; + struct handle *h = get_handle (conn, b->i); bool fua = !!(flags & NBDKIT_FLAG_FUA); bool fast = !!(flags & NBDKIT_FLAG_FAST_ZERO); int r; @@ -601,7 +601,7 @@ backend_extents (struct backend *b, struct nbdkit_extents *extents, int *err) { GET_CONN; - struct b_conn_handle *h = &conn->handles[b->i]; + struct handle *h = get_handle (conn, b->i); int r; assert (h->handle && (h->state & HANDLE_CONNECTED)); @@ -632,7 +632,7 @@ backend_cache (struct backend *b, uint32_t flags, int *err) { GET_CONN; - struct b_conn_handle *h = &conn->handles[b->i]; + struct handle *h = get_handle (conn, b->i); int r; assert (h->handle && (h->state & HANDLE_CONNECTED)); diff --git a/server/connections.c b/server/connections.c index 7e9584b3..d4cdf994 100644 --- a/server/connections.c +++ b/server/connections.c @@ -58,15 +58,6 @@ static int raw_send_socket (const void *buf, size_t len, int flags); static int raw_send_other (const void *buf, size_t len, int flags); static void raw_close (void); -void * -connection_get_handle (size_t i) -{ - GET_CONN; - - assert (i < conn->nr_handles); - return conn->handles[i].handle; -} - int connection_get_status (void) { @@ -258,7 +249,7 @@ new_connection (int sockin, int sockout, int nworkers) } conn->nr_handles = top->i + 1; for_each_backend (b) - reset_b_conn_handle (&conn->handles[b->i]); + reset_handle (get_handle (conn, b->i)); conn->status = 1; conn->nworkers = nworkers; -- 2.25.0
Richard W.M. Jones
2020-Feb-12 13:40 UTC
[Libguestfs] [PATCH nbdkit 3/3] server: filters: Remove struct b_h.
This was previously used as ‘nxdata’ and stored a tuple of ’b->next’ and the real filter handle. However after recent changes we don't need it. We can use ‘b->next’ as nxdata, and the handle is passed to us by the calling functions. Inspired by Eric Blakes observations in this email: https://www.redhat.com/archives/libguestfs/2020-February/msg00092.html --- server/filters.c | 217 ++++++++++++++++------------------------------- 1 file changed, 73 insertions(+), 144 deletions(-) diff --git a/server/filters.c b/server/filters.c index c916217c..92b0ceb3 100644 --- a/server/filters.c +++ b/server/filters.c @@ -49,16 +49,6 @@ struct backend_filter { struct nbdkit_filter filter; }; -/* Literally a backend and the filter's handle. - * - * This is the implementation of our handle in .open, and serves as - * a stable ‘void *nxdata’ in the filter API. - */ -struct b_h { - struct backend *b; - void *handle; -}; - /* Note this frees the whole chain. */ static void filter_free (struct backend *b) @@ -186,20 +176,19 @@ filter_config_complete (struct backend *b) static int next_preconnect (void *nxdata, int readonly) { - struct b_h *b_h = nxdata; - return b_h->b->preconnect (b_h->b, readonly); + struct backend *b_next = nxdata; + return b_next->preconnect (b_next, readonly); } static int filter_preconnect (struct backend *b, int readonly) { struct backend_filter *f = container_of (b, struct backend_filter, backend); - struct b_h nxdata = { .b = b->next }; debug ("%s: preconnect", b->name); if (f->filter.preconnect) - return f->filter.preconnect (next_preconnect, &nxdata, readonly); + return f->filter.preconnect (next_preconnect, b->next, readonly); else return b->next->preconnect (b->next, readonly); } @@ -216,201 +205,181 @@ plugin_magic_config_key (struct backend *b) static int next_open (void *nxdata, int readonly) { - struct b_h *b_h = nxdata; + struct backend *b_next = nxdata; - return backend_open (b_h->b, readonly); + return backend_open (b_next, readonly); } static void * filter_open (struct backend *b, int readonly) { struct backend_filter *f = container_of (b, struct backend_filter, backend); - struct b_h *nxdata = malloc (sizeof *nxdata); - - if (!nxdata) { - nbdkit_error ("malloc: %m"); - return NULL; - } - - nxdata->b = b->next; - nxdata->handle = NULL; + void *handle; /* Most filters will call next_open first, resulting in * inner-to-outer ordering. */ if (f->filter.open) - nxdata->handle = f->filter.open (next_open, nxdata, readonly); + handle = f->filter.open (next_open, b->next, readonly); else if (backend_open (b->next, readonly) == -1) - nxdata->handle = NULL; + handle = NULL; else - nxdata->handle = NBDKIT_HANDLE_NOT_NEEDED; - if (nxdata->handle == NULL) { - free (nxdata); - return NULL; - } - return nxdata; + handle = NBDKIT_HANDLE_NOT_NEEDED; + return handle; } static void filter_close (struct backend *b, void *handle) { struct backend_filter *f = container_of (b, struct backend_filter, backend); - struct b_h *nxdata = handle; - if (handle && f->filter.close) { - assert (nxdata->b == b->next); - f->filter.close (nxdata->handle); - free (nxdata); - } + if (handle && f->filter.close) + f->filter.close (handle); } /* The next_functions structure contains pointers to backend - * functions. However because these functions are all expecting a - * backend and a handle, we cannot call them directly, but must - * write some next_* functions that unpack the two parameters from a - * single ‘void *nxdata’ struct pointer (‘b_h’). + * functions. These are only needed for type safety (nxdata is void + * pointer, backend_* functions expect a struct backend * parameter). + * nxdata is a pointer to the next backend in the linked list. */ static int next_reopen (void *nxdata, int readonly) { - struct b_h *b_h = nxdata; - return backend_reopen (b_h->b, readonly); + struct backend *b_next = nxdata; + return backend_reopen (b_next, readonly); } static int64_t next_get_size (void *nxdata) { - struct b_h *b_h = nxdata; - return backend_get_size (b_h->b); + struct backend *b_next = nxdata; + return backend_get_size (b_next); } static int next_can_write (void *nxdata) { - struct b_h *b_h = nxdata; - return backend_can_write (b_h->b); + struct backend *b_next = nxdata; + return backend_can_write (b_next); } static int next_can_flush (void *nxdata) { - struct b_h *b_h = nxdata; - return backend_can_flush (b_h->b); + struct backend *b_next = nxdata; + return backend_can_flush (b_next); } static int next_is_rotational (void *nxdata) { - struct b_h *b_h = nxdata; - return backend_is_rotational (b_h->b); + struct backend *b_next = nxdata; + return backend_is_rotational (b_next); } static int next_can_trim (void *nxdata) { - struct b_h *b_h = nxdata; - return backend_can_trim (b_h->b); + struct backend *b_next = nxdata; + return backend_can_trim (b_next); } static int next_can_zero (void *nxdata) { - struct b_h *b_h = nxdata; - return backend_can_zero (b_h->b); + struct backend *b_next = nxdata; + return backend_can_zero (b_next); } static int next_can_fast_zero (void *nxdata) { - struct b_h *b_h = nxdata; - return backend_can_fast_zero (b_h->b); + struct backend *b_next = nxdata; + return backend_can_fast_zero (b_next); } static int next_can_extents (void *nxdata) { - struct b_h *b_h = nxdata; - return backend_can_extents (b_h->b); + struct backend *b_next = nxdata; + return backend_can_extents (b_next); } static int next_can_fua (void *nxdata) { - struct b_h *b_h = nxdata; - return backend_can_fua (b_h->b); + struct backend *b_next = nxdata; + return backend_can_fua (b_next); } static int next_can_multi_conn (void *nxdata) { - struct b_h *b_h = nxdata; - return backend_can_multi_conn (b_h->b); + struct backend *b_next = nxdata; + return backend_can_multi_conn (b_next); } static int next_can_cache (void *nxdata) { - struct b_h *b_h = nxdata; - return backend_can_cache (b_h->b); + struct backend *b_next = nxdata; + return backend_can_cache (b_next); } static int next_pread (void *nxdata, void *buf, uint32_t count, uint64_t offset, uint32_t flags, int *err) { - struct b_h *b_h = nxdata; - return backend_pread (b_h->b, buf, count, offset, flags, - err); + struct backend *b_next = nxdata; + return backend_pread (b_next, buf, count, offset, flags, err); } static int next_pwrite (void *nxdata, const void *buf, uint32_t count, uint64_t offset, uint32_t flags, int *err) { - struct b_h *b_h = nxdata; - return backend_pwrite (b_h->b, buf, count, offset, flags, - err); + struct backend *b_next = nxdata; + return backend_pwrite (b_next, buf, count, offset, flags, err); } static int next_flush (void *nxdata, uint32_t flags, int *err) { - struct b_h *b_h = nxdata; - return backend_flush (b_h->b, flags, err); + struct backend *b_next = nxdata; + return backend_flush (b_next, flags, err); } static int next_trim (void *nxdata, uint32_t count, uint64_t offset, uint32_t flags, int *err) { - struct b_h *b_h = nxdata; - return backend_trim (b_h->b, count, offset, flags, err); + struct backend *b_next = nxdata; + return backend_trim (b_next, count, offset, flags, err); } static int next_zero (void *nxdata, uint32_t count, uint64_t offset, uint32_t flags, int *err) { - struct b_h *b_h = nxdata; - return backend_zero (b_h->b, count, offset, flags, err); + struct backend *b_next = nxdata; + return backend_zero (b_next, count, offset, flags, err); } static int next_extents (void *nxdata, uint32_t count, uint64_t offset, uint32_t flags, struct nbdkit_extents *extents, int *err) { - struct b_h *b_h = nxdata; - return backend_extents (b_h->b, count, offset, flags, - extents, err); + struct backend *b_next = nxdata; + return backend_extents (b_next, count, offset, flags, extents, err); } static int next_cache (void *nxdata, uint32_t count, uint64_t offset, uint32_t flags, int *err) { - struct b_h *b_h = nxdata; - return backend_cache (b_h->b, count, offset, flags, err); + struct backend *b_next = nxdata; + return backend_cache (b_next, count, offset, flags, err); } static struct nbdkit_next_ops next_ops = { @@ -439,11 +408,9 @@ static int filter_prepare (struct backend *b, void *handle, int readonly) { struct backend_filter *f = container_of (b, struct backend_filter, backend); - struct b_h *nxdata = handle; - assert (nxdata->b == b->next); if (f->filter.prepare && - f->filter.prepare (&next_ops, nxdata, nxdata->handle, readonly) == -1) + f->filter.prepare (&next_ops, b->next, handle, readonly) == -1) return -1; return 0; @@ -453,11 +420,9 @@ static int filter_finalize (struct backend *b, void *handle) { struct backend_filter *f = container_of (b, struct backend_filter, backend); - struct b_h *nxdata = handle; - assert (nxdata->b == b->next); if (f->filter.finalize && - f->filter.finalize (&next_ops, nxdata, nxdata->handle) == -1) + f->filter.finalize (&next_ops, b->next, handle) == -1) return -1; return 0; } @@ -466,11 +431,9 @@ static int64_t filter_get_size (struct backend *b, void *handle) { struct backend_filter *f = container_of (b, struct backend_filter, backend); - struct b_h *nxdata = handle; - assert (nxdata->b == b->next); if (f->filter.get_size) - return f->filter.get_size (&next_ops, nxdata, nxdata->handle); + return f->filter.get_size (&next_ops, b->next, handle); else return backend_get_size (b->next); } @@ -479,11 +442,9 @@ static int filter_can_write (struct backend *b, void *handle) { struct backend_filter *f = container_of (b, struct backend_filter, backend); - struct b_h *nxdata = handle; - assert (nxdata->b == b->next); if (f->filter.can_write) - return f->filter.can_write (&next_ops, nxdata, nxdata->handle); + return f->filter.can_write (&next_ops, b->next, handle); else return backend_can_write (b->next); } @@ -492,11 +453,9 @@ static int filter_can_flush (struct backend *b, void *handle) { struct backend_filter *f = container_of (b, struct backend_filter, backend); - struct b_h *nxdata = handle; - assert (nxdata->b == b->next); if (f->filter.can_flush) - return f->filter.can_flush (&next_ops, nxdata, nxdata->handle); + return f->filter.can_flush (&next_ops, b->next, handle); else return backend_can_flush (b->next); } @@ -505,11 +464,9 @@ static int filter_is_rotational (struct backend *b, void *handle) { struct backend_filter *f = container_of (b, struct backend_filter, backend); - struct b_h *nxdata = handle; - assert (nxdata->b == b->next); if (f->filter.is_rotational) - return f->filter.is_rotational (&next_ops, nxdata, nxdata->handle); + return f->filter.is_rotational (&next_ops, b->next, handle); else return backend_is_rotational (b->next); } @@ -518,11 +475,9 @@ static int filter_can_trim (struct backend *b, void *handle) { struct backend_filter *f = container_of (b, struct backend_filter, backend); - struct b_h *nxdata = handle; - assert (nxdata->b == b->next); if (f->filter.can_trim) - return f->filter.can_trim (&next_ops, nxdata, nxdata->handle); + return f->filter.can_trim (&next_ops, b->next, handle); else return backend_can_trim (b->next); } @@ -531,11 +486,9 @@ static int filter_can_zero (struct backend *b, void *handle) { struct backend_filter *f = container_of (b, struct backend_filter, backend); - struct b_h *nxdata = handle; - assert (nxdata->b == b->next); if (f->filter.can_zero) - return f->filter.can_zero (&next_ops, nxdata, nxdata->handle); + return f->filter.can_zero (&next_ops, b->next, handle); else return backend_can_zero (b->next); } @@ -544,11 +497,9 @@ static int filter_can_fast_zero (struct backend *b, void *handle) { struct backend_filter *f = container_of (b, struct backend_filter, backend); - struct b_h *nxdata = handle; - assert (nxdata->b == b->next); if (f->filter.can_fast_zero) - return f->filter.can_fast_zero (&next_ops, nxdata, nxdata->handle); + return f->filter.can_fast_zero (&next_ops, b->next, handle); else return backend_can_fast_zero (b->next); } @@ -557,11 +508,9 @@ static int filter_can_extents (struct backend *b, void *handle) { struct backend_filter *f = container_of (b, struct backend_filter, backend); - struct b_h *nxdata = handle; - assert (nxdata->b == b->next); if (f->filter.can_extents) - return f->filter.can_extents (&next_ops, nxdata, nxdata->handle); + return f->filter.can_extents (&next_ops, b->next, handle); else return backend_can_extents (b->next); } @@ -570,11 +519,9 @@ static int filter_can_fua (struct backend *b, void *handle) { struct backend_filter *f = container_of (b, struct backend_filter, backend); - struct b_h *nxdata = handle; - assert (nxdata->b == b->next); if (f->filter.can_fua) - return f->filter.can_fua (&next_ops, nxdata, nxdata->handle); + return f->filter.can_fua (&next_ops, b->next, handle); else return backend_can_fua (b->next); } @@ -583,11 +530,9 @@ static int filter_can_multi_conn (struct backend *b, void *handle) { struct backend_filter *f = container_of (b, struct backend_filter, backend); - struct b_h *nxdata = handle; - assert (nxdata->b == b->next); if (f->filter.can_multi_conn) - return f->filter.can_multi_conn (&next_ops, nxdata, nxdata->handle); + return f->filter.can_multi_conn (&next_ops, b->next, handle); else return backend_can_multi_conn (b->next); } @@ -596,11 +541,9 @@ static int filter_can_cache (struct backend *b, void *handle) { struct backend_filter *f = container_of (b, struct backend_filter, backend); - struct b_h *nxdata = handle; - assert (nxdata->b == b->next); if (f->filter.can_cache) - return f->filter.can_cache (&next_ops, nxdata, nxdata->handle); + return f->filter.can_cache (&next_ops, b->next, handle); else return backend_can_cache (b->next); } @@ -611,11 +554,9 @@ filter_pread (struct backend *b, void *handle, uint32_t flags, int *err) { struct backend_filter *f = container_of (b, struct backend_filter, backend); - struct b_h *nxdata = handle; - assert (nxdata->b == b->next); if (f->filter.pread) - return f->filter.pread (&next_ops, nxdata, nxdata->handle, + return f->filter.pread (&next_ops, b->next, handle, buf, count, offset, flags, err); else return backend_pread (b->next, buf, count, offset, flags, err); @@ -627,11 +568,9 @@ filter_pwrite (struct backend *b, void *handle, uint32_t flags, int *err) { struct backend_filter *f = container_of (b, struct backend_filter, backend); - struct b_h *nxdata = handle; - assert (nxdata->b == b->next); if (f->filter.pwrite) - return f->filter.pwrite (&next_ops, nxdata, nxdata->handle, + return f->filter.pwrite (&next_ops, b->next, handle, buf, count, offset, flags, err); else return backend_pwrite (b->next, buf, count, offset, flags, err); @@ -642,11 +581,9 @@ filter_flush (struct backend *b, void *handle, uint32_t flags, int *err) { struct backend_filter *f = container_of (b, struct backend_filter, backend); - struct b_h *nxdata = handle; - assert (nxdata->b == b->next); if (f->filter.flush) - return f->filter.flush (&next_ops, nxdata, nxdata->handle, flags, err); + return f->filter.flush (&next_ops, b->next, handle, flags, err); else return backend_flush (b->next, flags, err); } @@ -657,11 +594,9 @@ filter_trim (struct backend *b, void *handle, uint32_t flags, int *err) { struct backend_filter *f = container_of (b, struct backend_filter, backend); - struct b_h *nxdata = handle; - assert (nxdata->b == b->next); if (f->filter.trim) - return f->filter.trim (&next_ops, nxdata, nxdata->handle, count, offset, + return f->filter.trim (&next_ops, b->next, handle, count, offset, flags, err); else return backend_trim (b->next, count, offset, flags, err); @@ -672,11 +607,9 @@ filter_zero (struct backend *b, void *handle, uint32_t count, uint64_t offset, uint32_t flags, int *err) { struct backend_filter *f = container_of (b, struct backend_filter, backend); - struct b_h *nxdata = handle; - assert (nxdata->b == b->next); if (f->filter.zero) - return f->filter.zero (&next_ops, nxdata, nxdata->handle, + return f->filter.zero (&next_ops, b->next, handle, count, offset, flags, err); else return backend_zero (b->next, count, offset, flags, err); @@ -688,11 +621,9 @@ filter_extents (struct backend *b, void *handle, struct nbdkit_extents *extents, int *err) { struct backend_filter *f = container_of (b, struct backend_filter, backend); - struct b_h *nxdata = handle; - assert (nxdata->b == b->next); if (f->filter.extents) - return f->filter.extents (&next_ops, nxdata, nxdata->handle, + return f->filter.extents (&next_ops, b->next, handle, count, offset, flags, extents, err); else @@ -706,11 +637,9 @@ filter_cache (struct backend *b, void *handle, uint32_t flags, int *err) { struct backend_filter *f = container_of (b, struct backend_filter, backend); - struct b_h *nxdata = handle; - assert (nxdata->b == b->next); if (f->filter.cache) - return f->filter.cache (&next_ops, nxdata, nxdata->handle, + return f->filter.cache (&next_ops, b->next, handle, count, offset, flags, err); else return backend_cache (b->next, count, offset, flags, err); -- 2.25.0
Eric Blake
2020-Feb-12 13:42 UTC
Re: [Libguestfs] [PATCH nbdkit 1/3] server: Rename global backend pointer to "top".
On 2/12/20 7:40 AM, Richard W.M. Jones wrote:> It's confusing to use the same terminology for a single backend as for > the linked list of backends. In particular it's often not clear if > we're calling the next backend or the whole chain of backends.Yes, that has caught me, too. I like the rename. ACK> +++ b/server/internal.h > @@ -118,8 +118,18 @@ extern char *unixsocket; > extern const char *user, *group; > extern bool verbose; > > -extern struct backend *backend; > -#define for_each_backend(b) for (b = backend; b != NULL; b = b->next) > +/* Linked list of backends. Each backend struct is followed by either > + * a filter or plugin struct. "top" points to the first one. They > + * are linked through the backend->next field. > + * > + * ┌──────────┐ ┌──────────┐ ┌──────────┐ > + * top ───▶│ backend │───▶│ backend │───▶│ backend │ > + * │ b->i = 2 │ │ b->i = 1 │ │ b->i = 0 │ > + * │ filter │ │ filter │ │ plugin │ > + * └──────────┘ └──────────┘ └──────────┘ > + */And the graphic helps, too. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org
Eric Blake
2020-Feb-12 13:45 UTC
Re: [Libguestfs] [PATCH nbdkit 2/3] server: Rename ‘struct b_conn_handle’ to plain ‘struct handle’.
On 2/12/20 7:40 AM, Richard W.M. Jones wrote:> Also add an inline accessor function (get_handle). > > This also removes the unused function connection_get_handle. > --- > server/internal.h | 24 ++++++++++++++++----- > server/backend.c | 50 ++++++++++++++++++++++---------------------- > server/connections.c | 11 +--------- > 3 files changed, 45 insertions(+), 40 deletions(-)ACK. I'm not sure when I removed the last use of connection_get_handle, or why I didn't clean it up back then. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org
Eric Blake
2020-Feb-12 13:56 UTC
Re: [Libguestfs] [PATCH nbdkit 3/3] server: filters: Remove struct b_h.
On 2/12/20 7:40 AM, Richard W.M. Jones wrote:> This was previously used as ‘nxdata’ and stored a tuple of ’b->next’ > and the real filter handle. However after recent changes we don't > need it. We can use ‘b->next’ as nxdata, and the handle is passed to > us by the calling functions. > > Inspired by Eric Blakes observations in this email:Blake's> https://www.redhat.com/archives/libguestfs/2020-February/msg00092.html > --- > server/filters.c | 217 ++++++++++++++++------------------------------- > 1 file changed, 73 insertions(+), 144 deletions(-) >> @@ -216,201 +205,181 @@ plugin_magic_config_key (struct backend *b) > static int > next_open (void *nxdata, int readonly) > { > - struct b_h *b_h = nxdata; > + struct backend *b_next = nxdata; > > - return backend_open (b_h->b, readonly); > + return backend_open (b_next, readonly); > }With this change, 'next_open' and '(int (*)(void *, int))backend_open' now have identical semantics. I'm trying to see if there are further changes we could make that would alleviate the need for function casts. I don't know if it is worth changing nbdkit-filter.h to use 'struct backend *' instead of 'void *' (while leaving struct backend an incomplete type in the public header) - it would be ABI compatible, and although it would require recompilation, we already state that filter recompilation is par for the course (since only plugins promise API compatibility). But one step at a time; your patch is fine as-is. ACK> /* The next_functions structure contains pointers to backend > - * functions. However because these functions are all expecting a > - * backend and a handle, we cannot call them directly, but must > - * write some next_* functions that unpack the two parameters from a > - * single ‘void *nxdata’ struct pointer (‘b_h’). > + * functions. These are only needed for type safety (nxdata is void > + * pointer, backend_* functions expect a struct backend * parameter). > + * nxdata is a pointer to the next backend in the linked list. > */ >-- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org
Reasonably Related Threads
- [PATCH nbdkit v2 0/3] server: Remove explicit connection parameter.
- [PATCH nbdkit 0/3] server: Remove explicit connection parameter.
- [nbdkit PATCH 0/5] More retry fixes
- Re: [PATCH nbdkit 3/3] server: Remove explicit connection parameter, use TLS instead.
- [PATCH nbdkit 3/3] server: Remove explicit connection parameter, use TLS instead.