Anywhere that we compute 'f = container_of(b)' then use 'f->backend.', we can instead just use 'b->'. Similarly, during registration, we can use the 'filename' input rather than our just-copied 'f->filename'. Signed-off-by: Eric Blake <eblake(a)redhat.com&gt; --- server/filters.c | 170 ++++++++++++++++++++++------------------------- 1 file changed, 79 insertions(+), 91 deletions(-) diff --git a/server/filters.c b/server/filters.c index bb6394fb..287c8747 100644 --- a/server/filters.c +++ b/server/filters.c @@ -68,7 +68,7 @@ filter_free (struct backend *b) { struct backend_filter *f = container_of (b, struct backend_filter, backend); - f->backend.next->free (f->backend.next); + b->next->free (b->next); /* Acquiring this lock prevents any filter callbacks from running * simultaneously. @@ -94,7 +94,7 @@ filter_thread_model (struct backend *b) { struct backend_filter *f = container_of (b, struct backend_filter, backend); int filter_thread_model = NBDKIT_THREAD_MODEL_PARALLEL; - int thread_model = f->backend.next->thread_model (f->backend.next); + int thread_model = b->next->thread_model (b->next); if (f->filter.thread_model) { filter_thread_model = f->filter.thread_model (); @@ -114,9 +114,7 @@ filter_thread_model (struct backend *b) static const char * plugin_name (struct backend *b) { - struct backend_filter *f = container_of (b, struct backend_filter, backend); - - return f->backend.next->plugin_name (f->backend.next); + return b->next->plugin_name (b->next); } static const char * @@ -161,9 +159,7 @@ filter_usage (struct backend *b) static void filter_dump_fields (struct backend *b) { - struct backend_filter *f = container_of (b, struct backend_filter, backend); - - f->backend.next->dump_fields (f->backend.next); + b->next->dump_fields (b->next); } static int @@ -183,11 +179,11 @@ filter_config (struct backend *b, const char *key, const char *value) f->name, key, value); if (f->filter.config) { - if (f->filter.config (next_config, f->backend.next, key, value) == -1) + if (f->filter.config (next_config, b->next, key, value) == -1) exit (EXIT_FAILURE); } else - f->backend.next->config (f->backend.next, key, value); + b->next->config (b->next, key, value); } static int @@ -206,11 +202,11 @@ filter_config_complete (struct backend *b) debug ("%s: config_complete", f->name); if (f->filter.config_complete) { - if (f->filter.config_complete (next_config_complete, f->backend.next) == -1) + if (f->filter.config_complete (next_config_complete, b->next) == -1) exit (EXIT_FAILURE); } else - f->backend.next->config_complete (f->backend.next); + b->next->config_complete (b->next); } /* magic_config_key only applies to plugins, so this passes the @@ -219,9 +215,7 @@ filter_config_complete (struct backend *b) static const char * plugin_magic_config_key (struct backend *b) { - struct backend_filter *f = container_of (b, struct backend_filter, backend); - - return f->backend.next->magic_config_key (f->backend.next); + return b->next->magic_config_key (b->next); } static int @@ -236,7 +230,7 @@ static int filter_open (struct backend *b, struct connection *conn, int readonly) { struct backend_filter *f = container_of (b, struct backend_filter, backend); - struct b_conn nxdata = { .b = f->backend.next, .conn = conn }; + struct b_conn nxdata = { .b = b->next, .conn = conn }; void *handle; debug ("%s: open readonly=%d", f->name, readonly); @@ -245,24 +239,24 @@ filter_open (struct backend *b, struct connection *conn, int readonly) handle = f->filter.open (next_open, &nxdata, readonly); if (handle == NULL) return -1; - connection_set_handle (conn, f->backend.i, handle); + connection_set_handle (conn, b->i, handle); return 0; } else - return f->backend.next->open (f->backend.next, conn, readonly); + return b->next->open (b->next, conn, readonly); } static void filter_close (struct backend *b, struct connection *conn) { struct backend_filter *f = container_of (b, struct backend_filter, backend); - void *handle = connection_get_handle (conn, f->backend.i); + void *handle = connection_get_handle (conn, b->i); debug ("%s: close", f->name); if (f->filter.close) f->filter.close (handle); - f->backend.next->close (f->backend.next, conn); + b->next->close (b->next, conn); } /* The next_functions structure contains pointers to backend @@ -459,15 +453,15 @@ static int filter_prepare (struct backend *b, struct connection *conn) { struct backend_filter *f = container_of (b, struct backend_filter, backend); - void *handle = connection_get_handle (conn, f->backend.i); - struct b_conn nxdata = { .b = f->backend.next, .conn = conn }; + void *handle = connection_get_handle (conn, b->i); + struct b_conn nxdata = { .b = b->next, .conn = conn }; debug ("%s: prepare", f->name); /* Call these in order starting from the filter closest to the * plugin. */ - if (f->backend.next->prepare (f->backend.next, conn) == -1) + if (b->next->prepare (b->next, conn) == -1) return -1; if (f->filter.prepare && @@ -481,8 +475,8 @@ static int filter_finalize (struct backend *b, struct connection *conn) { struct backend_filter *f = container_of (b, struct backend_filter, backend); - void *handle = connection_get_handle (conn, f->backend.i); - struct b_conn nxdata = { .b = f->backend.next, .conn = conn }; + void *handle = connection_get_handle (conn, b->i); + struct b_conn nxdata = { .b = b->next, .conn = conn }; debug ("%s: finalize", f->name); @@ -493,157 +487,157 @@ filter_finalize (struct backend *b, struct connection *conn) f->filter.finalize (&next_ops, &nxdata, handle) == -1) return -1; - return f->backend.next->finalize (f->backend.next, conn); + return b->next->finalize (b->next, conn); } static int64_t filter_get_size (struct backend *b, struct connection *conn) { struct backend_filter *f = container_of (b, struct backend_filter, backend); - void *handle = connection_get_handle (conn, f->backend.i); - struct b_conn nxdata = { .b = f->backend.next, .conn = conn }; + void *handle = connection_get_handle (conn, b->i); + struct b_conn nxdata = { .b = b->next, .conn = conn }; debug ("%s: get_size", f->name); if (f->filter.get_size) return f->filter.get_size (&next_ops, &nxdata, handle); else - return f->backend.next->get_size (f->backend.next, conn); + return b->next->get_size (b->next, conn); } static int filter_can_write (struct backend *b, struct connection *conn) { struct backend_filter *f = container_of (b, struct backend_filter, backend); - void *handle = connection_get_handle (conn, f->backend.i); - struct b_conn nxdata = { .b = f->backend.next, .conn = conn }; + void *handle = connection_get_handle (conn, b->i); + struct b_conn nxdata = { .b = b->next, .conn = conn }; debug ("%s: can_write", f->name); if (f->filter.can_write) return f->filter.can_write (&next_ops, &nxdata, handle); else - return f->backend.next->can_write (f->backend.next, conn); + return b->next->can_write (b->next, conn); } static int filter_can_flush (struct backend *b, struct connection *conn) { struct backend_filter *f = container_of (b, struct backend_filter, backend); - void *handle = connection_get_handle (conn, f->backend.i); - struct b_conn nxdata = { .b = f->backend.next, .conn = conn }; + void *handle = connection_get_handle (conn, b->i); + struct b_conn nxdata = { .b = b->next, .conn = conn }; debug ("%s: can_flush", f->name); if (f->filter.can_flush) return f->filter.can_flush (&next_ops, &nxdata, handle); else - return f->backend.next->can_flush (f->backend.next, conn); + return b->next->can_flush (b->next, conn); } static int filter_is_rotational (struct backend *b, struct connection *conn) { struct backend_filter *f = container_of (b, struct backend_filter, backend); - void *handle = connection_get_handle (conn, f->backend.i); - struct b_conn nxdata = { .b = f->backend.next, .conn = conn }; + void *handle = connection_get_handle (conn, b->i); + struct b_conn nxdata = { .b = b->next, .conn = conn }; debug ("%s: is_rotational", f->name); if (f->filter.is_rotational) return f->filter.is_rotational (&next_ops, &nxdata, handle); else - return f->backend.next->is_rotational (f->backend.next, conn); + return b->next->is_rotational (b->next, conn); } static int filter_can_trim (struct backend *b, struct connection *conn) { struct backend_filter *f = container_of (b, struct backend_filter, backend); - void *handle = connection_get_handle (conn, f->backend.i); - struct b_conn nxdata = { .b = f->backend.next, .conn = conn }; + void *handle = connection_get_handle (conn, b->i); + struct b_conn nxdata = { .b = b->next, .conn = conn }; debug ("%s: can_trim", f->name); if (f->filter.can_trim) return f->filter.can_trim (&next_ops, &nxdata, handle); else - return f->backend.next->can_trim (f->backend.next, conn); + return b->next->can_trim (b->next, conn); } static int filter_can_zero (struct backend *b, struct connection *conn) { struct backend_filter *f = container_of (b, struct backend_filter, backend); - void *handle = connection_get_handle (conn, f->backend.i); - struct b_conn nxdata = { .b = f->backend.next, .conn = conn }; + void *handle = connection_get_handle (conn, b->i); + struct b_conn nxdata = { .b = b->next, .conn = conn }; debug ("%s: can_zero", f->name); if (f->filter.can_zero) return f->filter.can_zero (&next_ops, &nxdata, handle); else - return f->backend.next->can_zero (f->backend.next, conn); + return b->next->can_zero (b->next, conn); } static int filter_can_extents (struct backend *b, struct connection *conn) { struct backend_filter *f = container_of (b, struct backend_filter, backend); - void *handle = connection_get_handle (conn, f->backend.i); - struct b_conn nxdata = { .b = f->backend.next, .conn = conn }; + void *handle = connection_get_handle (conn, b->i); + struct b_conn nxdata = { .b = b->next, .conn = conn }; debug ("%s: can_extents", f->name); if (f->filter.can_extents) return f->filter.can_extents (&next_ops, &nxdata, handle); else - return f->backend.next->can_extents (f->backend.next, conn); + return b->next->can_extents (b->next, conn); } static int filter_can_fua (struct backend *b, struct connection *conn) { struct backend_filter *f = container_of (b, struct backend_filter, backend); - void *handle = connection_get_handle (conn, f->backend.i); - struct b_conn nxdata = { .b = f->backend.next, .conn = conn }; + void *handle = connection_get_handle (conn, b->i); + struct b_conn nxdata = { .b = b->next, .conn = conn }; debug ("%s: can_fua", f->name); if (f->filter.can_fua) return f->filter.can_fua (&next_ops, &nxdata, handle); else - return f->backend.next->can_fua (f->backend.next, conn); + return b->next->can_fua (b->next, conn); } static int filter_can_multi_conn (struct backend *b, struct connection *conn) { struct backend_filter *f = container_of (b, struct backend_filter, backend); - void *handle = connection_get_handle (conn, f->backend.i); - struct b_conn nxdata = { .b = f->backend.next, .conn = conn }; + void *handle = connection_get_handle (conn, b->i); + struct b_conn nxdata = { .b = b->next, .conn = conn }; debug ("%s: can_multi_conn", f->name); if (f->filter.can_multi_conn) return f->filter.can_multi_conn (&next_ops, &nxdata, handle); else - return f->backend.next->can_multi_conn (f->backend.next, conn); + return b->next->can_multi_conn (b->next, conn); } static int filter_can_cache (struct backend *b, struct connection *conn) { struct backend_filter *f = container_of (b, struct backend_filter, backend); - void *handle = connection_get_handle (conn, f->backend.i); - struct b_conn nxdata = { .b = f->backend.next, .conn = conn }; + void *handle = connection_get_handle (conn, b->i); + struct b_conn nxdata = { .b = b->next, .conn = conn }; debug ("%s: can_cache", f->name); if (f->filter.can_cache) return f->filter.can_cache (&next_ops, &nxdata, handle); else - return f->backend.next->can_cache (f->backend.next, conn); + return b->next->can_cache (b->next, conn); } static int @@ -652,8 +646,8 @@ filter_pread (struct backend *b, struct connection *conn, uint32_t flags, int *err) { struct backend_filter *f = container_of (b, struct backend_filter, backend); - void *handle = connection_get_handle (conn, f->backend.i); - struct b_conn nxdata = { .b = f->backend.next, .conn = conn }; + void *handle = connection_get_handle (conn, b->i); + struct b_conn nxdata = { .b = b->next, .conn = conn }; assert (flags == 0); @@ -664,8 +658,7 @@ filter_pread (struct backend *b, struct connection *conn, return f->filter.pread (&next_ops, &nxdata, handle, buf, count, offset, flags, err); else - return f->backend.next->pread (f->backend.next, conn, - buf, count, offset, flags, err); + return b->next->pread (b->next, conn, buf, count, offset, flags, err); } static int @@ -674,8 +667,8 @@ filter_pwrite (struct backend *b, struct connection *conn, uint32_t flags, int *err) { struct backend_filter *f = container_of (b, struct backend_filter, backend); - void *handle = connection_get_handle (conn, f->backend.i); - struct b_conn nxdata = { .b = f->backend.next, .conn = conn }; + void *handle = connection_get_handle (conn, b->i); + struct b_conn nxdata = { .b = b->next, .conn = conn }; assert (!(flags & ~NBDKIT_FLAG_FUA)); @@ -686,8 +679,7 @@ filter_pwrite (struct backend *b, struct connection *conn, return f->filter.pwrite (&next_ops, &nxdata, handle, buf, count, offset, flags, err); else - return f->backend.next->pwrite (f->backend.next, conn, - buf, count, offset, flags, err); + return b->next->pwrite (b->next, conn, buf, count, offset, flags, err); } static int @@ -695,8 +687,8 @@ filter_flush (struct backend *b, struct connection *conn, uint32_t flags, int *err) { struct backend_filter *f = container_of (b, struct backend_filter, backend); - void *handle = connection_get_handle (conn, f->backend.i); - struct b_conn nxdata = { .b = f->backend.next, .conn = conn }; + void *handle = connection_get_handle (conn, b->i); + struct b_conn nxdata = { .b = b->next, .conn = conn }; assert (flags == 0); @@ -705,7 +697,7 @@ filter_flush (struct backend *b, struct connection *conn, uint32_t flags, if (f->filter.flush) return f->filter.flush (&next_ops, &nxdata, handle, flags, err); else - return f->backend.next->flush (f->backend.next, conn, flags, err); + return b->next->flush (b->next, conn, flags, err); } static int @@ -714,8 +706,8 @@ filter_trim (struct backend *b, struct connection *conn, uint32_t flags, int *err) { struct backend_filter *f = container_of (b, struct backend_filter, backend); - void *handle = connection_get_handle (conn, f->backend.i); - struct b_conn nxdata = { .b = f->backend.next, .conn = conn }; + void *handle = connection_get_handle (conn, b->i); + struct b_conn nxdata = { .b = b->next, .conn = conn }; assert (flags == 0); @@ -726,8 +718,7 @@ filter_trim (struct backend *b, struct connection *conn, return f->filter.trim (&next_ops, &nxdata, handle, count, offset, flags, err); else - return f->backend.next->trim (f->backend.next, conn, count, offset, flags, - err); + return b->next->trim (b->next, conn, count, offset, flags, err); } static int @@ -735,8 +726,8 @@ filter_zero (struct backend *b, struct connection *conn, uint32_t count, uint64_t offset, uint32_t flags, int *err) { struct backend_filter *f = container_of (b, struct backend_filter, backend); - void *handle = connection_get_handle (conn, f->backend.i); - struct b_conn nxdata = { .b = f->backend.next, .conn = conn }; + void *handle = connection_get_handle (conn, b->i); + struct b_conn nxdata = { .b = b->next, .conn = conn }; assert (!(flags & ~(NBDKIT_FLAG_MAY_TRIM | NBDKIT_FLAG_FUA))); @@ -747,8 +738,7 @@ filter_zero (struct backend *b, struct connection *conn, return f->filter.zero (&next_ops, &nxdata, handle, count, offset, flags, err); else - return f->backend.next->zero (f->backend.next, conn, - count, offset, flags, err); + return b->next->zero (b->next, conn, count, offset, flags, err); } static int @@ -757,8 +747,8 @@ filter_extents (struct backend *b, struct connection *conn, struct nbdkit_extents *extents, int *err) { struct backend_filter *f = container_of (b, struct backend_filter, backend); - void *handle = connection_get_handle (conn, f->backend.i); - struct b_conn nxdata = { .b = f->backend.next, .conn = conn }; + void *handle = connection_get_handle (conn, b->i); + struct b_conn nxdata = { .b = b->next, .conn = conn }; assert (!(flags & ~NBDKIT_FLAG_REQ_ONE)); @@ -770,9 +760,8 @@ filter_extents (struct backend *b, struct connection *conn, count, offset, flags, extents, err); else - return f->backend.next->extents (f->backend.next, conn, - count, offset, flags, - extents, err); + return b->next->extents (b->next, conn, count, offset, flags, + extents, err); } static int @@ -781,8 +770,8 @@ filter_cache (struct backend *b, struct connection *conn, uint32_t flags, int *err) { struct backend_filter *f = container_of (b, struct backend_filter, backend); - void *handle = connection_get_handle (conn, f->backend.i); - struct b_conn nxdata = { .b = f->backend.next, .conn = conn }; + void *handle = connection_get_handle (conn, b->i); + struct b_conn nxdata = { .b = b->next, .conn = conn }; assert (flags == 0); @@ -793,8 +782,7 @@ filter_cache (struct backend *b, struct connection *conn, return f->filter.cache (&next_ops, &nxdata, handle, count, offset, flags, err); else - return f->backend.next->cache (f->backend.next, conn, - count, offset, flags, err); + return b->next->cache (b->next, conn, count, offset, flags, err); } static struct backend filter_functions = { @@ -858,7 +846,7 @@ filter_register (struct backend *next, size_t index, const char *filename, if (f->filename == NULL) goto out_of_memory; f->dl = dl; - debug ("registering filter %s", f->filename); + debug ("registering filter %s", filename); /* Call the initialization function which returns the address of the * filter's own 'struct nbdkit_filter'. @@ -866,7 +854,7 @@ filter_register (struct backend *next, size_t index, const char *filename, filter = filter_init (); if (!filter) { fprintf (stderr, "%s: %s: filter registration function failed\n", - program_name, f->filename); + program_name, filename); exit (EXIT_FAILURE); } @@ -887,14 +875,14 @@ filter_register (struct backend *next, size_t index, const char *filename, fprintf (stderr, "%s: %s: filter is incompatible with this version of nbdkit, " "and appears to stem from nbdkit 1.14 or earlier\n", - program_name, f->filename); + program_name, filename); exit (EXIT_FAILURE); } if (strcmp (filter->_nbdkit_version, NBDKIT_VERSION_STRING) != 0) { fprintf (stderr, "%s: %s: filter is incompatible with this version of nbdkit " "(_nbdkit_version = %s)\n", - program_name, f->filename, filter->_nbdkit_version); + program_name, filename, filter->_nbdkit_version); exit (EXIT_FAILURE); } @@ -903,14 +891,14 @@ filter_register (struct backend *next, size_t index, const char *filename, /* Only filter.name is required. */ if (f->filter.name == NULL) { fprintf (stderr, "%s: %s: filter must have a .name field\n", - program_name, f->filename); + program_name, filename); exit (EXIT_FAILURE); } len = strlen (f->filter.name); if (len == 0) { fprintf (stderr, "%s: %s: filter.name field must not be empty\n", - program_name, f->filename); + program_name, filename); exit (EXIT_FAILURE); } for (i = 0; i < len; ++i) { @@ -920,7 +908,7 @@ filter_register (struct backend *next, size_t index, const char *filename, fprintf (stderr, "%s: %s: filter.name ('%s') field " "must contain only ASCII alphanumeric characters\n", - program_name, f->filename, f->filter.name); + program_name, filename, f->filter.name); exit (EXIT_FAILURE); } } -- 2.21.0

Both plugin.c and filter.c add the same three fields on top of struct backend, and use very similar code in initializing them. Let's stop the duplication, by moving those three fields into struct backend, and creating a new backend.c for manipulating them. In turn, we can drop the backend->name() accessor in favor of just directly accessing the name field. This is a net reduction in lines of code (remember, the diffstat is also counting comment additions, including license boilerplate). Signed-off-by: Eric Blake <eblake(a)redhat.com&gt; --- server/internal.h | 21 +++++++- server/Makefile.am | 3 +- server/backend.c | 121 ++++++++++++++++++++++++++++++++++++++++++ server/filters.c | 129 +++++++++++---------------------------------- server/main.c | 2 +- server/plugins.c | 102 ++++++++--------------------------- 6 files changed, 195 insertions(+), 183 deletions(-) create mode 100644 server/backend.c diff --git a/server/internal.h b/server/internal.h index a9692bbc..3af6ca16 100644 --- a/server/internal.h +++ b/server/internal.h @@ -242,6 +242,7 @@ extern void log_stderr_verror (const char *fs, va_list args) extern void log_syslog_verror (const char *fs, va_list args) __attribute__((__format__ (printf, 1, 0))); +/* backend.c */ struct backend { /* Next filter or plugin in the chain. This is always NULL for * plugins and never NULL for filters. @@ -255,9 +256,18 @@ struct backend { */ size_t i; + /* A copy of the backend name that survives a dlclose. */ + char *name; + + /* The file the backend was loaded from. */ + char *filename; + + /* The dlopen handle for the backend. */ + void *dl; + + /* Backend callbacks. All are required. */ void (*free) (struct backend *); int (*thread_model) (struct backend *); - const char *(*name) (struct backend *); const char *(*plugin_name) (struct backend *); void (*usage) (struct backend *); const char *(*version) (struct backend *); @@ -298,6 +308,15 @@ struct backend { uint64_t offset, uint32_t flags, int *err); }; +extern void backend_init (struct backend *b, struct backend *next, size_t index, + const char *filename, void *dl, const char *type) + __attribute__((__nonnull__ (1, 4, 5, 6))); +extern void backend_set_name (struct backend *b, const char *name, + const char *type) + __attribute__((__nonnull__ (1, 3))); +extern void backend_unload (struct backend *b, void (*unload) (void)) + __attribute__((__nonnull__ (1))); + /* plugins.c */ extern struct backend *plugin_register (size_t index, const char *filename, void *dl, struct nbdkit_plugin *(*plugin_init) (void)) diff --git a/server/Makefile.am b/server/Makefile.am index 29674866..a0417639 100644 --- a/server/Makefile.am +++ b/server/Makefile.am @@ -1,5 +1,5 @@ # nbdkit -# Copyright (C) 2013-2018 Red Hat Inc. +# Copyright (C) 2013-2019 Red Hat Inc. # # Redistribution and use in source and binary forms, with or without # modification, are permitted provided that the following conditions are @@ -36,6 +36,7 @@ EXTRA_DIST = nbdkit.syms sbin_PROGRAMS = nbdkit nbdkit_SOURCES = \ + backend.c \ background.c \ captive.c \ connections.c \ diff --git a/server/backend.c b/server/backend.c new file mode 100644 index 00000000..c7ee2d05 --- /dev/null +++ b/server/backend.c @@ -0,0 +1,121 @@ +/* nbdkit + * Copyright (C) 2013-2019 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 <ctype.h> +#include <dlfcn.h> +#include <stdio.h> +#include <string.h> + +#include "internal.h" + +/* Helpers for registering a new backend. */ + +void +backend_init (struct backend *b, struct backend *next, size_t index, + const char *filename, void *dl, const char *type) +{ + b->next = next; + b->i = index; + b->filename = strdup (filename); + if (b->filename == NULL) { + perror ("strdup"); + exit (EXIT_FAILURE); + } + b->dl = dl; + + debug ("registering %s %s", type, filename); +} + +void +backend_set_name (struct backend *b, const char *name, const char *type) +{ + size_t i, len; + + /* name is required. */ + if (name == NULL) { + fprintf (stderr, "%s: %s: %s must have a .name field\n", + program_name, b->filename, type); + exit (EXIT_FAILURE); + } + + len = strlen (name); + if (len == 0) { + fprintf (stderr, "%s: %s: %s.name field must not be empty\n", + program_name, b->filename, type); + exit (EXIT_FAILURE); + } + for (i = 0; i < len; ++i) { + unsigned char c = name[i]; + + if (!(isascii (c) && isalnum (c))) { + fprintf (stderr, + "%s: %s: %s.name ('%s') field " + "must contain only ASCII alphanumeric characters\n", + program_name, b->filename, type, name); + exit (EXIT_FAILURE); + } + } + + /* Copy the module's name into local storage, so that name + * survives past unload. + */ + b->name = strdup (name); + if (b->name == NULL) { + perror ("strdup"); + exit (EXIT_FAILURE); + } + + debug ("registered %s %s (name %s)", type, b->filename, b->name); +} + +void +backend_unload (struct backend *b, void (*unload) (void)) +{ + /* Acquiring this lock prevents any other backend callbacks from running + * simultaneously. + */ + lock_unload (); + + debug ("%s: unload", b->name); + if (unload) + unload (); + + if (DO_DLCLOSE) + dlclose (b->dl); + free (b->filename); + + unlock_unload (); + + free (b->name); +} diff --git a/server/filters.c b/server/filters.c index 287c8747..1f76bf61 100644 --- a/server/filters.c +++ b/server/filters.c @@ -48,9 +48,6 @@ */ struct backend_filter { struct backend backend; - char *name; /* copy of filter.name */ - char *filename; - void *dl; struct nbdkit_filter filter; }; @@ -70,22 +67,7 @@ filter_free (struct backend *b) b->next->free (b->next); - /* Acquiring this lock prevents any filter callbacks from running - * simultaneously. - */ - lock_unload (); - - debug ("%s: unload", f->name); - if (f->filter.unload) - f->filter.unload (); - - if (DO_DLCLOSE) - dlclose (f->dl); - free (f->filename); - - unlock_unload (); - - free (f->name); + backend_unload (b, f->filter.unload); free (f); } @@ -117,14 +99,6 @@ plugin_name (struct backend *b) return b->next->plugin_name (b->next); } -static const char * -filter_name (struct backend *b) -{ - struct backend_filter *f = container_of (b, struct backend_filter, backend); - - return f->name; -} - static const char * filter_version (struct backend *b) { @@ -139,11 +113,11 @@ filter_usage (struct backend *b) struct backend_filter *f = container_of (b, struct backend_filter, backend); const char *p; - printf ("filter: %s", f->name); + printf ("filter: %s", b->name); if (f->filter.longname) printf (" (%s)", f->filter.longname); printf ("\n"); - printf ("(%s)\n", f->filename); + printf ("(%s)\n", b->filename); if (f->filter.description) { printf ("%s", f->filter.description); if ((p = strrchr (f->filter.description, '\n')) == NULL || p[1]) @@ -176,7 +150,7 @@ filter_config (struct backend *b, const char *key, const char *value) struct backend_filter *f = container_of (b, struct backend_filter, backend); debug ("%s: config key=%s, value=%s", - f->name, key, value); + b->name, key, value); if (f->filter.config) { if (f->filter.config (next_config, b->next, key, value) == -1) @@ -199,7 +173,7 @@ filter_config_complete (struct backend *b) { struct backend_filter *f = container_of (b, struct backend_filter, backend); - debug ("%s: config_complete", f->name); + debug ("%s: config_complete", b->name); if (f->filter.config_complete) { if (f->filter.config_complete (next_config_complete, b->next) == -1) @@ -233,7 +207,7 @@ filter_open (struct backend *b, struct connection *conn, int readonly) struct b_conn nxdata = { .b = b->next, .conn = conn }; void *handle; - debug ("%s: open readonly=%d", f->name, readonly); + debug ("%s: open readonly=%d", b->name, readonly); if (f->filter.open) { handle = f->filter.open (next_open, &nxdata, readonly); @@ -252,7 +226,7 @@ filter_close (struct backend *b, struct connection *conn) struct backend_filter *f = container_of (b, struct backend_filter, backend); void *handle = connection_get_handle (conn, b->i); - debug ("%s: close", f->name); + debug ("%s: close", b->name); if (f->filter.close) f->filter.close (handle); @@ -456,7 +430,7 @@ filter_prepare (struct backend *b, struct connection *conn) void *handle = connection_get_handle (conn, b->i); struct b_conn nxdata = { .b = b->next, .conn = conn }; - debug ("%s: prepare", f->name); + debug ("%s: prepare", b->name); /* Call these in order starting from the filter closest to the * plugin. @@ -478,7 +452,7 @@ filter_finalize (struct backend *b, struct connection *conn) void *handle = connection_get_handle (conn, b->i); struct b_conn nxdata = { .b = b->next, .conn = conn }; - debug ("%s: finalize", f->name); + debug ("%s: finalize", b->name); /* Call these in reverse order to .prepare above, starting from the * filter furthest away from the plugin. @@ -497,7 +471,7 @@ filter_get_size (struct backend *b, struct connection *conn) void *handle = connection_get_handle (conn, b->i); struct b_conn nxdata = { .b = b->next, .conn = conn }; - debug ("%s: get_size", f->name); + debug ("%s: get_size", b->name); if (f->filter.get_size) return f->filter.get_size (&next_ops, &nxdata, handle); @@ -512,7 +486,7 @@ filter_can_write (struct backend *b, struct connection *conn) void *handle = connection_get_handle (conn, b->i); struct b_conn nxdata = { .b = b->next, .conn = conn }; - debug ("%s: can_write", f->name); + debug ("%s: can_write", b->name); if (f->filter.can_write) return f->filter.can_write (&next_ops, &nxdata, handle); @@ -527,7 +501,7 @@ filter_can_flush (struct backend *b, struct connection *conn) void *handle = connection_get_handle (conn, b->i); struct b_conn nxdata = { .b = b->next, .conn = conn }; - debug ("%s: can_flush", f->name); + debug ("%s: can_flush", b->name); if (f->filter.can_flush) return f->filter.can_flush (&next_ops, &nxdata, handle); @@ -542,7 +516,7 @@ filter_is_rotational (struct backend *b, struct connection *conn) void *handle = connection_get_handle (conn, b->i); struct b_conn nxdata = { .b = b->next, .conn = conn }; - debug ("%s: is_rotational", f->name); + debug ("%s: is_rotational", b->name); if (f->filter.is_rotational) return f->filter.is_rotational (&next_ops, &nxdata, handle); @@ -557,7 +531,7 @@ filter_can_trim (struct backend *b, struct connection *conn) void *handle = connection_get_handle (conn, b->i); struct b_conn nxdata = { .b = b->next, .conn = conn }; - debug ("%s: can_trim", f->name); + debug ("%s: can_trim", b->name); if (f->filter.can_trim) return f->filter.can_trim (&next_ops, &nxdata, handle); @@ -572,7 +546,7 @@ filter_can_zero (struct backend *b, struct connection *conn) void *handle = connection_get_handle (conn, b->i); struct b_conn nxdata = { .b = b->next, .conn = conn }; - debug ("%s: can_zero", f->name); + debug ("%s: can_zero", b->name); if (f->filter.can_zero) return f->filter.can_zero (&next_ops, &nxdata, handle); @@ -587,7 +561,7 @@ filter_can_extents (struct backend *b, struct connection *conn) void *handle = connection_get_handle (conn, b->i); struct b_conn nxdata = { .b = b->next, .conn = conn }; - debug ("%s: can_extents", f->name); + debug ("%s: can_extents", b->name); if (f->filter.can_extents) return f->filter.can_extents (&next_ops, &nxdata, handle); @@ -602,7 +576,7 @@ filter_can_fua (struct backend *b, struct connection *conn) void *handle = connection_get_handle (conn, b->i); struct b_conn nxdata = { .b = b->next, .conn = conn }; - debug ("%s: can_fua", f->name); + debug ("%s: can_fua", b->name); if (f->filter.can_fua) return f->filter.can_fua (&next_ops, &nxdata, handle); @@ -617,7 +591,7 @@ filter_can_multi_conn (struct backend *b, struct connection *conn) void *handle = connection_get_handle (conn, b->i); struct b_conn nxdata = { .b = b->next, .conn = conn }; - debug ("%s: can_multi_conn", f->name); + debug ("%s: can_multi_conn", b->name); if (f->filter.can_multi_conn) return f->filter.can_multi_conn (&next_ops, &nxdata, handle); @@ -632,7 +606,7 @@ filter_can_cache (struct backend *b, struct connection *conn) void *handle = connection_get_handle (conn, b->i); struct b_conn nxdata = { .b = b->next, .conn = conn }; - debug ("%s: can_cache", f->name); + debug ("%s: can_cache", b->name); if (f->filter.can_cache) return f->filter.can_cache (&next_ops, &nxdata, handle); @@ -652,7 +626,7 @@ filter_pread (struct backend *b, struct connection *conn, assert (flags == 0); debug ("%s: pread count=%" PRIu32 " offset=%" PRIu64 " flags=0x%" PRIx32, - f->name, count, offset, flags); + b->name, count, offset, flags); if (f->filter.pread) return f->filter.pread (&next_ops, &nxdata, handle, @@ -673,7 +647,7 @@ filter_pwrite (struct backend *b, struct connection *conn, assert (!(flags & ~NBDKIT_FLAG_FUA)); debug ("%s: pwrite count=%" PRIu32 " offset=%" PRIu64 " flags=0x%" PRIx32, - f->name, count, offset, flags); + b->name, count, offset, flags); if (f->filter.pwrite) return f->filter.pwrite (&next_ops, &nxdata, handle, @@ -692,7 +666,7 @@ filter_flush (struct backend *b, struct connection *conn, uint32_t flags, assert (flags == 0); - debug ("%s: flush flags=0x%" PRIx32, f->name, flags); + debug ("%s: flush flags=0x%" PRIx32, b->name, flags); if (f->filter.flush) return f->filter.flush (&next_ops, &nxdata, handle, flags, err); @@ -712,7 +686,7 @@ filter_trim (struct backend *b, struct connection *conn, assert (flags == 0); debug ("%s: trim count=%" PRIu32 " offset=%" PRIu64 " flags=0x%" PRIx32, - f->name, count, offset, flags); + b->name, count, offset, flags); if (f->filter.trim) return f->filter.trim (&next_ops, &nxdata, handle, count, offset, flags, @@ -732,7 +706,7 @@ filter_zero (struct backend *b, struct connection *conn, assert (!(flags & ~(NBDKIT_FLAG_MAY_TRIM | NBDKIT_FLAG_FUA))); debug ("%s: zero count=%" PRIu32 " offset=%" PRIu64 " flags=0x%" PRIx32, - f->name, count, offset, flags); + b->name, count, offset, flags); if (f->filter.zero) return f->filter.zero (&next_ops, &nxdata, handle, @@ -753,7 +727,7 @@ filter_extents (struct backend *b, struct connection *conn, assert (!(flags & ~NBDKIT_FLAG_REQ_ONE)); debug ("%s: extents count=%" PRIu32 " offset=%" PRIu64 " flags=0x%" PRIx32, - f->name, count, offset, flags); + b->name, count, offset, flags); if (f->filter.extents) return f->filter.extents (&next_ops, &nxdata, handle, @@ -776,7 +750,7 @@ filter_cache (struct backend *b, struct connection *conn, assert (flags == 0); debug ("%s: cache count=%" PRIu32 " offset=%" PRIu64 " flags=0x%" PRIx32, - f->name, count, offset, flags); + b->name, count, offset, flags); if (f->filter.cache) return f->filter.cache (&next_ops, &nxdata, handle, @@ -788,7 +762,6 @@ filter_cache (struct backend *b, struct connection *conn, static struct backend filter_functions = { .free = filter_free, .thread_model = filter_thread_model, - .name = filter_name, .plugin_name = plugin_name, .usage = filter_usage, .version = filter_version, @@ -826,7 +799,6 @@ filter_register (struct backend *next, size_t index, const char *filename, { struct backend_filter *f; const struct nbdkit_filter *filter; - size_t i, len; union overlay { const char *str; unsigned int api; @@ -834,19 +806,12 @@ filter_register (struct backend *next, size_t index, const char *filename, f = calloc (1, sizeof *f); if (f == NULL) { - out_of_memory: perror ("strdup"); exit (EXIT_FAILURE); } f->backend = filter_functions; - f->backend.next = next; - f->backend.i = index; - f->filename = strdup (filename); - if (f->filename == NULL) goto out_of_memory; - f->dl = dl; - - debug ("registering filter %s", filename); + backend_init (&f->backend, next, index, filename, dl, "filter"); /* Call the initialization function which returns the address of the * filter's own 'struct nbdkit_filter'. @@ -888,47 +853,13 @@ filter_register (struct backend *next, size_t index, const char *filename, f->filter = *filter; - /* Only filter.name is required. */ - if (f->filter.name == NULL) { - fprintf (stderr, "%s: %s: filter must have a .name field\n", - program_name, filename); - exit (EXIT_FAILURE); - } - - len = strlen (f->filter.name); - if (len == 0) { - fprintf (stderr, "%s: %s: filter.name field must not be empty\n", - program_name, filename); - exit (EXIT_FAILURE); - } - for (i = 0; i < len; ++i) { - if (!((f->filter.name[i] >= '0' && f->filter.name[i] <= '9') || - (f->filter.name[i] >= 'a' && f->filter.name[i] <= 'z') || - (f->filter.name[i] >= 'A' && f->filter.name[i] <= 'Z'))) { - fprintf (stderr, - "%s: %s: filter.name ('%s') field " - "must contain only ASCII alphanumeric characters\n", - program_name, filename, f->filter.name); - exit (EXIT_FAILURE); - } - } - - /* Copy the module's name into local storage, so that filter.name - * survives past unload. - */ - f->name = strdup (f->filter.name); - if (f->name == NULL) { - perror ("strdup"); - exit (EXIT_FAILURE); - } - - debug ("registered filter %s (name %s)", f->filename, f->name); + backend_set_name (&f->backend, f->filter.name, "filter"); /* Set debug flags before calling load. */ - set_debug_flags (dl, f->name); + set_debug_flags (dl, f->backend.name); /* Call the on-load callback if it exists. */ - debug ("%s: load", f->name); + debug ("%s: load", f->backend.name); if (f->filter.load) f->filter.load (); diff --git a/server/main.c b/server/main.c index 124e19b7..6999818c 100644 --- a/server/main.c +++ b/server/main.c @@ -602,7 +602,7 @@ main (int argc, char *argv[]) display_version (); for_each_backend (b) { - printf ("%s", b->name (b)); + printf ("%s", b->name); if ((v = b->version (b)) != NULL) printf (" %s", v); printf ("\n"); diff --git a/server/plugins.c b/server/plugins.c index 34cc3cb5..0f70ede0 100644 --- a/server/plugins.c +++ b/server/plugins.c @@ -54,9 +54,6 @@ */ struct backend_plugin { struct backend backend; - char *name; /* copy of plugin.name */ - char *filename; - void *dl; struct nbdkit_plugin plugin; }; @@ -65,22 +62,7 @@ plugin_free (struct backend *b) { struct backend_plugin *p = container_of (b, struct backend_plugin, backend); - /* Acquiring this lock prevents any plugin callbacks from running - * simultaneously. - */ - lock_unload (); - - debug ("%s: unload", p->name); - if (p->plugin.unload) - p->plugin.unload (); - - if (DO_DLCLOSE) - dlclose (p->dl); - free (p->filename); - - unlock_unload (); - - free (p->name); + backend_unload (b, p->plugin.unload); free (p); } @@ -113,9 +95,7 @@ plugin_thread_model (struct backend *b) static const char * plugin_name (struct backend *b) { - struct backend_plugin *p = container_of (b, struct backend_plugin, backend); - - return p->name; + return b->name; } static void @@ -124,11 +104,11 @@ plugin_usage (struct backend *b) struct backend_plugin *p = container_of (b, struct backend_plugin, backend); const char *t; - printf ("plugin: %s", p->name); + printf ("plugin: %s", b->name); if (p->plugin.longname) printf (" (%s)", p->plugin.longname); printf ("\n"); - printf ("(%s)\n", p->filename); + printf ("(%s)\n", b->filename); if (p->plugin.description) { printf ("%s", p->plugin.description); if ((t = strrchr (p->plugin.description, '\n')) == NULL || t[1]) @@ -156,11 +136,11 @@ plugin_dump_fields (struct backend *b) struct backend_plugin *p = container_of (b, struct backend_plugin, backend); char *path; - path = nbdkit_realpath (p->filename); + path = nbdkit_realpath (b->filename); printf ("path=%s\n", path); free (path); - printf ("name=%s\n", p->name); + printf ("name=%s\n", b->name); if (p->plugin.version) printf ("version=%s\n", p->plugin.version); @@ -220,14 +200,14 @@ plugin_config (struct backend *b, const char *key, const char *value) { struct backend_plugin *p = container_of (b, struct backend_plugin, backend); - debug ("%s: config key=%s, value=%s", p->name, key, value); + debug ("%s: config key=%s, value=%s", b->name, key, value); if (p->plugin.config == NULL) { fprintf (stderr, "%s: %s: this plugin does not need command line configuration\n" "Try using: %s --help %s\n", - program_name, p->filename, - program_name, p->filename); + program_name, b->filename, + program_name, b->filename); exit (EXIT_FAILURE); } @@ -240,7 +220,7 @@ plugin_config_complete (struct backend *b) { struct backend_plugin *p = container_of (b, struct backend_plugin, backend); - debug ("%s: config_complete", p->name); + debug ("%s: config_complete", b->name); if (!p->plugin.config_complete) return; @@ -266,7 +246,7 @@ plugin_open (struct backend *b, struct connection *conn, int readonly) assert (connection_get_handle (conn, 0) == NULL); assert (p->plugin.open != NULL); - debug ("%s: open readonly=%d", p->name, readonly); + debug ("%s: open readonly=%d", b->name, readonly); handle = p->plugin.open (readonly); if (!handle) @@ -744,7 +724,6 @@ plugin_cache (struct backend *b, struct connection *conn, static struct backend plugin_functions = { .free = plugin_free, .thread_model = plugin_thread_model, - .name = plugin_name, .plugin_name = plugin_name, .usage = plugin_usage, .version = plugin_version, @@ -782,23 +761,16 @@ plugin_register (size_t index, const char *filename, { struct backend_plugin *p; const struct nbdkit_plugin *plugin; - size_t i, len, size; + size_t size; p = malloc (sizeof *p); if (p == NULL) { - out_of_memory: perror ("strdup"); exit (EXIT_FAILURE); } p->backend = plugin_functions; - p->backend.next = NULL; - p->backend.i = index; - p->filename = strdup (filename); - if (p->filename == NULL) goto out_of_memory; - p->dl = dl; - - debug ("registering plugin %s", p->filename); + backend_init (&p->backend, NULL, index, filename, dl, "plugin"); /* Call the initialization function which returns the address of the * plugin's own 'struct nbdkit_plugin'. @@ -806,7 +778,7 @@ plugin_register (size_t index, const char *filename, plugin = plugin_init (); if (!plugin) { fprintf (stderr, "%s: %s: plugin registration function failed\n", - program_name, p->filename); + program_name, filename); exit (EXIT_FAILURE); } @@ -815,7 +787,7 @@ plugin_register (size_t index, const char *filename, fprintf (stderr, "%s: %s: plugin is incompatible with this version of nbdkit " "(_api_version = %d)\n", - program_name, p->filename, plugin->_api_version); + program_name, filename, plugin->_api_version); exit (EXIT_FAILURE); } @@ -833,61 +805,29 @@ plugin_register (size_t index, const char *filename, /* Check for the minimum fields which must exist in the * plugin struct. */ - if (p->plugin.name == NULL) { - fprintf (stderr, "%s: %s: plugin must have a .name field\n", - program_name, p->filename); - exit (EXIT_FAILURE); - } if (p->plugin.open == NULL) { fprintf (stderr, "%s: %s: plugin must have a .open callback\n", - program_name, p->filename); + program_name, filename); exit (EXIT_FAILURE); } if (p->plugin.get_size == NULL) { fprintf (stderr, "%s: %s: plugin must have a .get_size callback\n", - program_name, p->filename); + program_name, filename); exit (EXIT_FAILURE); } if (p->plugin.pread == NULL && p->plugin._pread_old == NULL) { fprintf (stderr, "%s: %s: plugin must have a .pread callback\n", - program_name, p->filename); + program_name, filename); exit (EXIT_FAILURE); } - len = strlen (p->plugin.name); - if (len == 0) { - fprintf (stderr, "%s: %s: plugin.name field must not be empty\n", - program_name, p->filename); - exit (EXIT_FAILURE); - } - for (i = 0; i < len; ++i) { - if (!((p->plugin.name[i] >= '0' && p->plugin.name[i] <= '9') || - (p->plugin.name[i] >= 'a' && p->plugin.name[i] <= 'z') || - (p->plugin.name[i] >= 'A' && p->plugin.name[i] <= 'Z'))) { - fprintf (stderr, - "%s: %s: plugin.name ('%s') field " - "must contain only ASCII alphanumeric characters\n", - program_name, p->filename, p->plugin.name); - exit (EXIT_FAILURE); - } - } - - /* Copy the module's name into local storage, so that plugin.name - * survives past unload. - */ - p->name = strdup (p->plugin.name); - if (p->name == NULL) { - perror ("strdup"); - exit (EXIT_FAILURE); - } - - debug ("registered plugin %s (name %s)", p->filename, p->name); + backend_set_name (&p->backend, p->plugin.name, "plugin"); /* Set debug flags before calling load. */ - set_debug_flags (dl, p->name); + set_debug_flags (dl, p->backend.name); /* Call the on-load callback if it exists. */ - debug ("%s: load", p->name); + debug ("%s: load", p->backend.name); if (p->plugin.load) p->plugin.load (); -- 2.21.0

I'm planning to move sanity checking and per-connection caching into common code (right now, we check and cache things at the connection level for the final filter, but not at other layers; it would be nicer to do this at each step through the filters). This starts the process by extending the just-created backend.c to add wrappers for all of the handshake and request callbacks present in a filter's next_ops. This lets us consolidate common debugging output (which requires a tweak to test-layers), and moves assertion checking (such as what was duplicated in 0b8ad8c8) into a single location. Of note: .extents no longer returns 0 for a 0-length request made by a filter (such a request cannot be made by the client, because protocol.c:valid_range rejects it); commit 5a1db63d showed a case where a filter ended up doing that accidentally. Signed-off-by: Eric Blake <eblake(a)redhat.com&gt; --- server/internal.h | 49 +++++++ server/backend.c | 201 +++++++++++++++++++++++++++ server/filters.c | 158 +++++---------------- server/plugins.c | 48 ------- server/protocol-handshake-newstyle.c | 2 +- server/protocol-handshake-oldstyle.c | 2 +- server/protocol-handshake.c | 18 +-- server/protocol.c | 36 ++--- tests/test-layers.c | 50 +++---- 9 files changed, 331 insertions(+), 233 deletions(-) diff --git a/server/internal.h b/server/internal.h index 3af6ca16..93ebeb78 100644 --- a/server/internal.h +++ b/server/internal.h @@ -317,6 +317,55 @@ extern void backend_set_name (struct backend *b, const char *name, extern void backend_unload (struct backend *b, void (*unload) (void)) __attribute__((__nonnull__ (1))); +extern int64_t backend_get_size (struct backend *b, struct connection *conn) + __attribute__((__nonnull__ (1, 2))); +extern int backend_can_write (struct backend *b, struct connection *conn) + __attribute__((__nonnull__ (1, 2))); +extern int backend_can_flush (struct backend *b, struct connection *conn) + __attribute__((__nonnull__ (1, 2))); +extern int backend_is_rotational (struct backend *b, struct connection *conn) + __attribute__((__nonnull__ (1, 2))); +extern int backend_can_trim (struct backend *b, struct connection *conn) + __attribute__((__nonnull__ (1, 2))); +extern int backend_can_zero (struct backend *b, struct connection *conn) + __attribute__((__nonnull__ (1, 2))); +extern int backend_can_extents (struct backend *b, struct connection *conn) + __attribute__((__nonnull__ (1, 2))); +extern int backend_can_fua (struct backend *b, struct connection *conn) + __attribute__((__nonnull__ (1, 2))); +extern int backend_can_multi_conn (struct backend *b, struct connection *conn) + __attribute__((__nonnull__ (1, 2))); +extern int backend_can_cache (struct backend *b, struct connection *conn) + __attribute__((__nonnull__ (1, 2))); + +extern int backend_pread (struct backend *b, struct connection *conn, + void *buf, uint32_t count, uint64_t offset, + uint32_t flags, int *err) + __attribute__((__nonnull__ (1, 2, 3, 7))); +extern int backend_pwrite (struct backend *b, struct connection *conn, + const void *buf, uint32_t count, uint64_t offset, + uint32_t flags, int *err) + __attribute__((__nonnull__ (1, 2, 3, 7))); +extern int backend_flush (struct backend *b, struct connection *conn, + uint32_t flags, int *err) + __attribute__((__nonnull__ (1, 2, 4))); +extern int backend_trim (struct backend *b, struct connection *conn, + uint32_t count, uint64_t offset, uint32_t flags, + int *err) + __attribute__((__nonnull__ (1, 2, 6))); +extern int backend_zero (struct backend *b, struct connection *conn, + uint32_t count, uint64_t offset, uint32_t flags, + int *err) + __attribute__((__nonnull__ (1, 2, 6))); +extern int backend_extents (struct backend *b, struct connection *conn, + uint32_t count, uint64_t offset, uint32_t flags, + struct nbdkit_extents *extents, int *err) + __attribute__((__nonnull__ (1, 2, 6, 7))); +extern int backend_cache (struct backend *b, struct connection *conn, + uint32_t count, uint64_t offset, + uint32_t flags, int *err) + __attribute__((__nonnull__ (1, 2, 6))); + /* plugins.c */ extern struct backend *plugin_register (size_t index, const char *filename, void *dl, struct nbdkit_plugin *(*plugin_init) (void)) diff --git a/server/backend.c b/server/backend.c index c7ee2d05..ce3c5e2b 100644 --- a/server/backend.c +++ b/server/backend.c @@ -32,8 +32,10 @@ #include <config.h> +#include <assert.h> #include <ctype.h> #include <dlfcn.h> +#include <inttypes.h> #include <stdio.h> #include <string.h> @@ -119,3 +121,202 @@ backend_unload (struct backend *b, void (*unload) (void)) free (b->name); } + +int64_t +backend_get_size (struct backend *b, struct connection *conn) +{ + debug ("%s: get_size", b->name); + + /* TODO caching */ + return b->get_size (b, conn); +} + +int +backend_can_write (struct backend *b, struct connection *conn) +{ + debug ("%s: can_write", b->name); + + return b->can_write (b, conn); +} + +int +backend_can_flush (struct backend *b, struct connection *conn) +{ + debug ("%s: can_flush", b->name); + + return b->can_flush (b, conn); +} + +int +backend_is_rotational (struct backend *b, struct connection *conn) +{ + debug ("%s: is_rotational", b->name); + + return b->is_rotational (b, conn); +} + +int +backend_can_trim (struct backend *b, struct connection *conn) +{ + debug ("%s: can_trim", b->name); + + return b->can_trim (b, conn); +} + +int +backend_can_zero (struct backend *b, struct connection *conn) +{ + debug ("%s: can_zero", b->name); + + return b->can_zero (b, conn); +} + +int +backend_can_extents (struct backend *b, struct connection *conn) +{ + debug ("%s: can_extents", b->name); + + return b->can_extents (b, conn); +} + +int +backend_can_fua (struct backend *b, struct connection *conn) +{ + debug ("%s: can_fua", b->name); + + return b->can_fua (b, conn); +} + +int +backend_can_multi_conn (struct backend *b, struct connection *conn) +{ + debug ("%s: can_multi_conn", b->name); + + return b->can_multi_conn (b, conn); +} + +int +backend_can_cache (struct backend *b, struct connection *conn) +{ + debug ("%s: can_cache", b->name); + + return b->can_cache (b, conn); +} + +int +backend_pread (struct backend *b, struct connection *conn, + void *buf, uint32_t count, uint64_t offset, + uint32_t flags, int *err) +{ + int r; + + assert (flags == 0); + debug ("%s: pread count=%" PRIu32 " offset=%" PRIu64, + b->name, count, offset); + + r = b->pread (b, conn, buf, count, offset, flags, err); + if (r == -1) + assert (*err); + return r; +} + +int +backend_pwrite (struct backend *b, struct connection *conn, + const void *buf, uint32_t count, uint64_t offset, + uint32_t flags, int *err) +{ + int r; + + assert (!(flags & ~NBDKIT_FLAG_FUA)); + debug ("%s: pwrite count=%" PRIu32 " offset=%" PRIu64 " fua=%d", + b->name, count, offset, !!(flags & NBDKIT_FLAG_FUA)); + + r = b->pwrite (b, conn, buf, count, offset, flags, err); + if (r == -1) + assert (*err); + return r; +} + +int +backend_flush (struct backend *b, struct connection *conn, + uint32_t flags, int *err) +{ + int r; + + assert (flags == 0); + debug ("%s: flush", b->name); + + r = b->flush (b, conn, flags, err); + if (r == -1) + assert (*err); + return r; +} + +int +backend_trim (struct backend *b, struct connection *conn, + uint32_t count, uint64_t offset, uint32_t flags, + int *err) +{ + int r; + + assert (flags == 0); + debug ("%s: trim count=%" PRIu32 " offset=%" PRIu64 " fua=%d", + b->name, count, offset, !!(flags & NBDKIT_FLAG_FUA)); + + r = b->trim (b, conn, count, offset, flags, err); + if (r == -1) + assert (*err); + return r; +} + +int +backend_zero (struct backend *b, struct connection *conn, + uint32_t count, uint64_t offset, uint32_t flags, + int *err) +{ + int r; + + assert (!(flags & ~(NBDKIT_FLAG_MAY_TRIM | NBDKIT_FLAG_FUA))); + debug ("%s: zero count=%" PRIu32 " offset=%" PRIu64 " may_trim=%d fua=%d", + b->name, count, offset, !!(flags & NBDKIT_FLAG_MAY_TRIM), + !!(flags & NBDKIT_FLAG_FUA)); + + r = b->zero (b, conn, count, offset, flags, err); + if (r == -1) + assert (*err && *err != ENOTSUP && *err != EOPNOTSUPP); + return r; +} + +int +backend_extents (struct backend *b, struct connection *conn, + uint32_t count, uint64_t offset, uint32_t flags, + struct nbdkit_extents *extents, int *err) +{ + int r; + + assert (!(flags & ~NBDKIT_FLAG_REQ_ONE)); + debug ("%s: extents count=%" PRIu32 " offset=%" PRIu64 " req_one=%d", + b->name, count, offset, !!(flags & NBDKIT_FLAG_REQ_ONE)); + + r = b->extents (b, conn, count, offset, flags, extents, err); + if (r == -1) + assert (*err); + return r; +} + +int +backend_cache (struct backend *b, struct connection *conn, + uint32_t count, uint64_t offset, + uint32_t flags, int *err) +{ + int r; + + assert (flags == 0); + debug ("%s: cache count=%" PRIu32 " offset=%" PRIu64, + b->name, count, offset); + + r = b->cache (b, conn, count, offset, flags, err); + if (r == -1) + assert (*err); + return r; +} diff --git a/server/filters.c b/server/filters.c index 1f76bf61..acb44bda 100644 --- a/server/filters.c +++ b/server/filters.c @@ -244,70 +244,70 @@ static int64_t next_get_size (void *nxdata) { struct b_conn *b_conn = nxdata; - return b_conn->b->get_size (b_conn->b, b_conn->conn); + return backend_get_size (b_conn->b, b_conn->conn); } static int next_can_write (void *nxdata) { struct b_conn *b_conn = nxdata; - return b_conn->b->can_write (b_conn->b, b_conn->conn); + return backend_can_write (b_conn->b, b_conn->conn); } static int next_can_flush (void *nxdata) { struct b_conn *b_conn = nxdata; - return b_conn->b->can_flush (b_conn->b, b_conn->conn); + return backend_can_flush (b_conn->b, b_conn->conn); } static int next_is_rotational (void *nxdata) { struct b_conn *b_conn = nxdata; - return b_conn->b->is_rotational (b_conn->b, b_conn->conn); + return backend_is_rotational (b_conn->b, b_conn->conn); } static int next_can_trim (void *nxdata) { struct b_conn *b_conn = nxdata; - return b_conn->b->can_trim (b_conn->b, b_conn->conn); + return backend_can_trim (b_conn->b, b_conn->conn); } static int next_can_zero (void *nxdata) { struct b_conn *b_conn = nxdata; - return b_conn->b->can_zero (b_conn->b, b_conn->conn); + return backend_can_zero (b_conn->b, b_conn->conn); } static int next_can_extents (void *nxdata) { struct b_conn *b_conn = nxdata; - return b_conn->b->can_extents (b_conn->b, b_conn->conn); + return backend_can_extents (b_conn->b, b_conn->conn); } static int next_can_fua (void *nxdata) { struct b_conn *b_conn = nxdata; - return b_conn->b->can_fua (b_conn->b, b_conn->conn); + return backend_can_fua (b_conn->b, b_conn->conn); } static int next_can_multi_conn (void *nxdata) { struct b_conn *b_conn = nxdata; - return b_conn->b->can_multi_conn (b_conn->b, b_conn->conn); + return backend_can_multi_conn (b_conn->b, b_conn->conn); } static int next_can_cache (void *nxdata) { struct b_conn *b_conn = nxdata; - return b_conn->b->can_cache (b_conn->b, b_conn->conn); + return backend_can_cache (b_conn->b, b_conn->conn); } static int @@ -315,13 +315,8 @@ next_pread (void *nxdata, void *buf, uint32_t count, uint64_t offset, uint32_t flags, int *err) { struct b_conn *b_conn = nxdata; - int r; - - r = b_conn->b->pread (b_conn->b, b_conn->conn, buf, count, offset, flags, + return backend_pread (b_conn->b, b_conn->conn, buf, count, offset, flags, err); - if (r == -1) - assert (*err); - return r; } static int @@ -329,25 +324,15 @@ next_pwrite (void *nxdata, const void *buf, uint32_t count, uint64_t offset, uint32_t flags, int *err) { struct b_conn *b_conn = nxdata; - int r; - - r = b_conn->b->pwrite (b_conn->b, b_conn->conn, buf, count, offset, flags, + return backend_pwrite (b_conn->b, b_conn->conn, buf, count, offset, flags, err); - if (r == -1) - assert (*err); - return r; } static int next_flush (void *nxdata, uint32_t flags, int *err) { struct b_conn *b_conn = nxdata; - int r; - - r = b_conn->b->flush (b_conn->b, b_conn->conn, flags, err); - if (r == -1) - assert (*err); - return r; + return backend_flush (b_conn->b, b_conn->conn, flags, err); } static int @@ -355,12 +340,7 @@ next_trim (void *nxdata, uint32_t count, uint64_t offset, uint32_t flags, int *err) { struct b_conn *b_conn = nxdata; - int r; - - r = b_conn->b->trim (b_conn->b, b_conn->conn, count, offset, flags, err); - if (r == -1) - assert (*err); - return r; + return backend_trim (b_conn->b, b_conn->conn, count, offset, flags, err); } static int @@ -368,12 +348,7 @@ next_zero (void *nxdata, uint32_t count, uint64_t offset, uint32_t flags, int *err) { struct b_conn *b_conn = nxdata; - int r; - - r = b_conn->b->zero (b_conn->b, b_conn->conn, count, offset, flags, err); - if (r == -1) - assert (*err && *err != ENOTSUP && *err != EOPNOTSUPP); - return r; + return backend_zero (b_conn->b, b_conn->conn, count, offset, flags, err); } static int @@ -381,13 +356,8 @@ next_extents (void *nxdata, uint32_t count, uint64_t offset, uint32_t flags, struct nbdkit_extents *extents, int *err) { struct b_conn *b_conn = nxdata; - int r; - - r = b_conn->b->extents (b_conn->b, b_conn->conn, count, offset, flags, + return backend_extents (b_conn->b, b_conn->conn, count, offset, flags, extents, err); - if (r == -1) - assert (*err); - return r; } static int @@ -395,12 +365,7 @@ next_cache (void *nxdata, uint32_t count, uint64_t offset, uint32_t flags, int *err) { struct b_conn *b_conn = nxdata; - int r; - - r = b_conn->b->cache (b_conn->b, b_conn->conn, count, offset, flags, err); - if (r == -1) - assert (*err); - return r; + return backend_cache (b_conn->b, b_conn->conn, count, offset, flags, err); } static struct nbdkit_next_ops next_ops = { @@ -471,12 +436,10 @@ filter_get_size (struct backend *b, struct connection *conn) void *handle = connection_get_handle (conn, b->i); struct b_conn nxdata = { .b = b->next, .conn = conn }; - debug ("%s: get_size", b->name); - if (f->filter.get_size) return f->filter.get_size (&next_ops, &nxdata, handle); else - return b->next->get_size (b->next, conn); + return backend_get_size (b->next, conn); } static int @@ -486,12 +449,10 @@ filter_can_write (struct backend *b, struct connection *conn) void *handle = connection_get_handle (conn, b->i); struct b_conn nxdata = { .b = b->next, .conn = conn }; - debug ("%s: can_write", b->name); - if (f->filter.can_write) return f->filter.can_write (&next_ops, &nxdata, handle); else - return b->next->can_write (b->next, conn); + return backend_can_write (b->next, conn); } static int @@ -501,12 +462,10 @@ filter_can_flush (struct backend *b, struct connection *conn) void *handle = connection_get_handle (conn, b->i); struct b_conn nxdata = { .b = b->next, .conn = conn }; - debug ("%s: can_flush", b->name); - if (f->filter.can_flush) return f->filter.can_flush (&next_ops, &nxdata, handle); else - return b->next->can_flush (b->next, conn); + return backend_can_flush (b->next, conn); } static int @@ -516,12 +475,10 @@ filter_is_rotational (struct backend *b, struct connection *conn) void *handle = connection_get_handle (conn, b->i); struct b_conn nxdata = { .b = b->next, .conn = conn }; - debug ("%s: is_rotational", b->name); - if (f->filter.is_rotational) return f->filter.is_rotational (&next_ops, &nxdata, handle); else - return b->next->is_rotational (b->next, conn); + return backend_is_rotational (b->next, conn); } static int @@ -531,12 +488,10 @@ filter_can_trim (struct backend *b, struct connection *conn) void *handle = connection_get_handle (conn, b->i); struct b_conn nxdata = { .b = b->next, .conn = conn }; - debug ("%s: can_trim", b->name); - if (f->filter.can_trim) return f->filter.can_trim (&next_ops, &nxdata, handle); else - return b->next->can_trim (b->next, conn); + return backend_can_trim (b->next, conn); } static int @@ -546,12 +501,10 @@ filter_can_zero (struct backend *b, struct connection *conn) void *handle = connection_get_handle (conn, b->i); struct b_conn nxdata = { .b = b->next, .conn = conn }; - debug ("%s: can_zero", b->name); - if (f->filter.can_zero) return f->filter.can_zero (&next_ops, &nxdata, handle); else - return b->next->can_zero (b->next, conn); + return backend_can_zero (b->next, conn); } static int @@ -561,12 +514,10 @@ filter_can_extents (struct backend *b, struct connection *conn) void *handle = connection_get_handle (conn, b->i); struct b_conn nxdata = { .b = b->next, .conn = conn }; - debug ("%s: can_extents", b->name); - if (f->filter.can_extents) return f->filter.can_extents (&next_ops, &nxdata, handle); else - return b->next->can_extents (b->next, conn); + return backend_can_extents (b->next, conn); } static int @@ -576,12 +527,10 @@ filter_can_fua (struct backend *b, struct connection *conn) void *handle = connection_get_handle (conn, b->i); struct b_conn nxdata = { .b = b->next, .conn = conn }; - debug ("%s: can_fua", b->name); - if (f->filter.can_fua) return f->filter.can_fua (&next_ops, &nxdata, handle); else - return b->next->can_fua (b->next, conn); + return backend_can_fua (b->next, conn); } static int @@ -591,12 +540,10 @@ filter_can_multi_conn (struct backend *b, struct connection *conn) void *handle = connection_get_handle (conn, b->i); struct b_conn nxdata = { .b = b->next, .conn = conn }; - debug ("%s: can_multi_conn", b->name); - if (f->filter.can_multi_conn) return f->filter.can_multi_conn (&next_ops, &nxdata, handle); else - return b->next->can_multi_conn (b->next, conn); + return backend_can_multi_conn (b->next, conn); } static int @@ -606,12 +553,10 @@ filter_can_cache (struct backend *b, struct connection *conn) void *handle = connection_get_handle (conn, b->i); struct b_conn nxdata = { .b = b->next, .conn = conn }; - debug ("%s: can_cache", b->name); - if (f->filter.can_cache) return f->filter.can_cache (&next_ops, &nxdata, handle); else - return b->next->can_cache (b->next, conn); + return backend_can_cache (b->next, conn); } static int @@ -623,16 +568,11 @@ filter_pread (struct backend *b, struct connection *conn, void *handle = connection_get_handle (conn, b->i); struct b_conn nxdata = { .b = b->next, .conn = conn }; - assert (flags == 0); - - debug ("%s: pread count=%" PRIu32 " offset=%" PRIu64 " flags=0x%" PRIx32, - b->name, count, offset, flags); - if (f->filter.pread) return f->filter.pread (&next_ops, &nxdata, handle, buf, count, offset, flags, err); else - return b->next->pread (b->next, conn, buf, count, offset, flags, err); + return backend_pread (b->next, conn, buf, count, offset, flags, err); } static int @@ -644,16 +584,11 @@ filter_pwrite (struct backend *b, struct connection *conn, void *handle = connection_get_handle (conn, b->i); struct b_conn nxdata = { .b = b->next, .conn = conn }; - assert (!(flags & ~NBDKIT_FLAG_FUA)); - - debug ("%s: pwrite count=%" PRIu32 " offset=%" PRIu64 " flags=0x%" PRIx32, - b->name, count, offset, flags); - if (f->filter.pwrite) return f->filter.pwrite (&next_ops, &nxdata, handle, buf, count, offset, flags, err); else - return b->next->pwrite (b->next, conn, buf, count, offset, flags, err); + return backend_pwrite (b->next, conn, buf, count, offset, flags, err); } static int @@ -664,14 +599,10 @@ filter_flush (struct backend *b, struct connection *conn, uint32_t flags, void *handle = connection_get_handle (conn, b->i); struct b_conn nxdata = { .b = b->next, .conn = conn }; - assert (flags == 0); - - debug ("%s: flush flags=0x%" PRIx32, b->name, flags); - if (f->filter.flush) return f->filter.flush (&next_ops, &nxdata, handle, flags, err); else - return b->next->flush (b->next, conn, flags, err); + return backend_flush (b->next, conn, flags, err); } static int @@ -683,16 +614,11 @@ filter_trim (struct backend *b, struct connection *conn, void *handle = connection_get_handle (conn, b->i); struct b_conn nxdata = { .b = b->next, .conn = conn }; - assert (flags == 0); - - debug ("%s: trim count=%" PRIu32 " offset=%" PRIu64 " flags=0x%" PRIx32, - b->name, count, offset, flags); - if (f->filter.trim) return f->filter.trim (&next_ops, &nxdata, handle, count, offset, flags, err); else - return b->next->trim (b->next, conn, count, offset, flags, err); + return backend_trim (b->next, conn, count, offset, flags, err); } static int @@ -703,16 +629,11 @@ filter_zero (struct backend *b, struct connection *conn, void *handle = connection_get_handle (conn, b->i); struct b_conn nxdata = { .b = b->next, .conn = conn }; - assert (!(flags & ~(NBDKIT_FLAG_MAY_TRIM | NBDKIT_FLAG_FUA))); - - debug ("%s: zero count=%" PRIu32 " offset=%" PRIu64 " flags=0x%" PRIx32, - b->name, count, offset, flags); - if (f->filter.zero) return f->filter.zero (&next_ops, &nxdata, handle, count, offset, flags, err); else - return b->next->zero (b->next, conn, count, offset, flags, err); + return backend_zero (b->next, conn, count, offset, flags, err); } static int @@ -724,18 +645,13 @@ filter_extents (struct backend *b, struct connection *conn, void *handle = connection_get_handle (conn, b->i); struct b_conn nxdata = { .b = b->next, .conn = conn }; - assert (!(flags & ~NBDKIT_FLAG_REQ_ONE)); - - debug ("%s: extents count=%" PRIu32 " offset=%" PRIu64 " flags=0x%" PRIx32, - b->name, count, offset, flags); - if (f->filter.extents) return f->filter.extents (&next_ops, &nxdata, handle, count, offset, flags, extents, err); else - return b->next->extents (b->next, conn, count, offset, flags, - extents, err); + return backend_extents (b->next, conn, count, offset, flags, + extents, err); } static int @@ -747,16 +663,12 @@ filter_cache (struct backend *b, struct connection *conn, void *handle = connection_get_handle (conn, b->i); struct b_conn nxdata = { .b = b->next, .conn = conn }; - assert (flags == 0); - - debug ("%s: cache count=%" PRIu32 " offset=%" PRIu64 " flags=0x%" PRIx32, - b->name, count, offset, flags); if (f->filter.cache) return f->filter.cache (&next_ops, &nxdata, handle, count, offset, flags, err); else - return b->next->cache (b->next, conn, count, offset, flags, err); + return backend_cache (b->next, conn, count, offset, flags, err); } static struct backend filter_functions = { diff --git a/server/plugins.c b/server/plugins.c index 0f70ede0..da8931a3 100644 --- a/server/plugins.c +++ b/server/plugins.c @@ -295,8 +295,6 @@ plugin_get_size (struct backend *b, struct connection *conn) assert (connection_get_handle (conn, 0)); assert (p->plugin.get_size != NULL); - debug ("get_size"); - return p->plugin.get_size (connection_get_handle (conn, 0)); } @@ -307,8 +305,6 @@ plugin_can_write (struct backend *b, struct connection *conn) assert (connection_get_handle (conn, 0)); - debug ("can_write"); - if (p->plugin.can_write) return p->plugin.can_write (connection_get_handle (conn, 0)); else @@ -322,8 +318,6 @@ plugin_can_flush (struct backend *b, struct connection *conn) assert (connection_get_handle (conn, 0)); - debug ("can_flush"); - if (p->plugin.can_flush) return p->plugin.can_flush (connection_get_handle (conn, 0)); else @@ -337,8 +331,6 @@ plugin_is_rotational (struct backend *b, struct connection *conn) assert (connection_get_handle (conn, 0)); - debug ("is_rotational"); - if (p->plugin.is_rotational) return p->plugin.is_rotational (connection_get_handle (conn, 0)); else @@ -352,8 +344,6 @@ plugin_can_trim (struct backend *b, struct connection *conn) assert (connection_get_handle (conn, 0)); - debug ("can_trim"); - if (p->plugin.can_trim) return p->plugin.can_trim (connection_get_handle (conn, 0)); else @@ -367,8 +357,6 @@ plugin_can_zero (struct backend *b, struct connection *conn) assert (connection_get_handle (conn, 0)); - debug ("can_zero"); - /* Note the special case here: the plugin's .can_zero controls only * whether we call .zero; while the backend expects .can_zero to * return whether to advertise zero support. Since we ALWAYS know @@ -388,8 +376,6 @@ plugin_can_extents (struct backend *b, struct connection *conn) assert (connection_get_handle (conn, 0)); - debug ("can_extents"); - if (p->plugin.can_extents) return p->plugin.can_extents (connection_get_handle (conn, 0)); else @@ -404,8 +390,6 @@ plugin_can_fua (struct backend *b, struct connection *conn) assert (connection_get_handle (conn, 0)); - debug ("can_fua"); - /* The plugin must use API version 2 and have .can_fua return NBDKIT_FUA_NATIVE before we will pass the FUA flag on. */ if (p->plugin.can_fua) { @@ -427,8 +411,6 @@ plugin_can_multi_conn (struct backend *b, struct connection *conn) assert (connection_get_handle (conn, 0)); - debug ("can_multi_conn"); - if (p->plugin.can_multi_conn) return p->plugin.can_multi_conn (connection_get_handle (conn, 0)); else @@ -442,8 +424,6 @@ plugin_can_cache (struct backend *b, struct connection *conn) assert (connection_get_handle (conn, 0)); - debug ("can_cache"); - if (p->plugin.can_cache) return p->plugin.can_cache (connection_get_handle (conn, 0)); if (p->plugin.cache) @@ -482,9 +462,6 @@ plugin_pread (struct backend *b, struct connection *conn, assert (connection_get_handle (conn, 0)); assert (p->plugin.pread || p->plugin._pread_old); - assert (!flags); - - debug ("pread count=%" PRIu32 " offset=%" PRIu64, count, offset); if (p->plugin.pread) r = p->plugin.pread (connection_get_handle (conn, 0), buf, count, offset, @@ -507,8 +484,6 @@ plugin_flush (struct backend *b, struct connection *conn, uint32_t flags, assert (connection_get_handle (conn, 0)); assert (!flags); - debug ("flush"); - if (p->plugin.flush) r = p->plugin.flush (connection_get_handle (conn, 0), 0); else if (p->plugin._flush_old) @@ -533,10 +508,6 @@ plugin_pwrite (struct backend *b, struct connection *conn, bool need_flush = false; assert (connection_get_handle (conn, 0)); - assert (!(flags & ~NBDKIT_FLAG_FUA)); - - debug ("pwrite count=%" PRIu32 " offset=%" PRIu64 " fua=%d", count, offset, - fua); if (fua && plugin_can_fua (b, conn) != NBDKIT_FUA_NATIVE) { flags &= ~NBDKIT_FLAG_FUA; @@ -569,10 +540,6 @@ plugin_trim (struct backend *b, struct connection *conn, bool need_flush = false; assert (connection_get_handle (conn, 0)); - assert (!(flags & ~NBDKIT_FLAG_FUA)); - - debug ("trim count=%" PRIu32 " offset=%" PRIu64 " fua=%d", count, offset, - fua); if (fua && plugin_can_fua (b, conn) != NBDKIT_FUA_NATIVE) { flags &= ~NBDKIT_FLAG_FUA; @@ -606,10 +573,6 @@ plugin_zero (struct backend *b, struct connection *conn, int can_zero = 1; /* TODO cache this per-connection? */ assert (connection_get_handle (conn, 0)); - assert (!(flags & ~(NBDKIT_FLAG_MAY_TRIM | NBDKIT_FLAG_FUA))); - - debug ("zero count=%" PRIu32 " offset=%" PRIu64 " may_trim=%d fua=%d", - count, offset, may_trim, fua); if (fua && plugin_can_fua (b, conn) != NBDKIT_FUA_NATIVE) { flags &= ~NBDKIT_FLAG_FUA; @@ -670,21 +633,13 @@ plugin_extents (struct backend *b, struct connection *conn, struct nbdkit_extents *extents, int *err) { struct backend_plugin *p = container_of (b, struct backend_plugin, backend); - bool req_one = flags & NBDKIT_FLAG_REQ_ONE; int r; assert (connection_get_handle (conn, 0)); - assert (!(flags & ~NBDKIT_FLAG_REQ_ONE)); /* This should be true because plugin_can_extents checks it. */ assert (p->plugin.extents); - debug ("extents count=%" PRIu32 " offset=%" PRIu64 " req_one=%d", - count, offset, req_one); - - if (!count) - return 0; - r = p->plugin.extents (connection_get_handle (conn, 0), count, offset, flags, extents); if (r >= 0 && nbdkit_extents_count (extents) < 1) { @@ -706,9 +661,6 @@ plugin_cache (struct backend *b, struct connection *conn, int r; assert (connection_get_handle (conn, 0)); - assert (!flags); - - debug ("cache count=%" PRIu32 " offset=%" PRIu64, count, offset); /* A plugin may advertise caching but not provide .cache; in that * case, caching is explicitly a no-op. */ diff --git a/server/protocol-handshake-newstyle.c b/server/protocol-handshake-newstyle.c index 6ccb76f3..28817317 100644 --- a/server/protocol-handshake-newstyle.c +++ b/server/protocol-handshake-newstyle.c @@ -205,7 +205,7 @@ finish_newstyle_options (struct connection *conn) { int64_t r; - r = backend->get_size (backend, conn); + r = backend_get_size (backend, conn); if (r == -1) return -1; if (r < 0) { diff --git a/server/protocol-handshake-oldstyle.c b/server/protocol-handshake-oldstyle.c index 9fde1ca0..af3ac488 100644 --- a/server/protocol-handshake-oldstyle.c +++ b/server/protocol-handshake-oldstyle.c @@ -59,7 +59,7 @@ protocol_handshake_oldstyle (struct connection *conn) return -1; } - r = backend->get_size (backend, conn); + r = backend_get_size (backend, conn); if (r == -1) return -1; if (r < 0) { diff --git a/server/protocol-handshake.c b/server/protocol-handshake.c index 0f3bd280..4d12b3dc 100644 --- a/server/protocol-handshake.c +++ b/server/protocol-handshake.c @@ -52,7 +52,7 @@ protocol_compute_eflags (struct connection *conn, uint16_t *flags) uint16_t eflags = NBD_FLAG_HAS_FLAGS; int fl; - fl = backend->can_write (backend, conn); + fl = backend_can_write (backend, conn); if (fl == -1) return -1; if (readonly || !fl) { @@ -60,7 +60,7 @@ protocol_compute_eflags (struct connection *conn, uint16_t *flags) conn->readonly = true; } if (!conn->readonly) { - fl = backend->can_zero (backend, conn); + fl = backend_can_zero (backend, conn); if (fl == -1) return -1; if (fl) { @@ -68,7 +68,7 @@ protocol_compute_eflags (struct connection *conn, uint16_t *flags) conn->can_zero = true; } - fl = backend->can_trim (backend, conn); + fl = backend_can_trim (backend, conn); if (fl == -1) return -1; if (fl) { @@ -76,7 +76,7 @@ protocol_compute_eflags (struct connection *conn, uint16_t *flags) conn->can_trim = true; } - fl = backend->can_fua (backend, conn); + fl = backend_can_fua (backend, conn); if (fl == -1) return -1; if (fl) { @@ -85,7 +85,7 @@ protocol_compute_eflags (struct connection *conn, uint16_t *flags) } } - fl = backend->can_flush (backend, conn); + fl = backend_can_flush (backend, conn); if (fl == -1) return -1; if (fl) { @@ -93,7 +93,7 @@ protocol_compute_eflags (struct connection *conn, uint16_t *flags) conn->can_flush = true; } - fl = backend->is_rotational (backend, conn); + fl = backend_is_rotational (backend, conn); if (fl == -1) return -1; if (fl) { @@ -104,7 +104,7 @@ protocol_compute_eflags (struct connection *conn, uint16_t *flags) /* multi-conn is useless if parallel connections are not allowed */ if (backend->thread_model (backend) > NBDKIT_THREAD_MODEL_SERIALIZE_CONNECTIONS) { - fl = backend->can_multi_conn (backend, conn); + fl = backend_can_multi_conn (backend, conn); if (fl == -1) return -1; if (fl) { @@ -113,7 +113,7 @@ protocol_compute_eflags (struct connection *conn, uint16_t *flags) } } - fl = backend->can_cache (backend, conn); + fl = backend_can_cache (backend, conn); if (fl == -1) return -1; if (fl) { @@ -127,7 +127,7 @@ protocol_compute_eflags (struct connection *conn, uint16_t *flags) * per connection and store the result in the handle anyway. This * protocol_compute_eflags function is a bit misnamed XXX. */ - fl = backend->can_extents (backend, conn); + fl = backend_can_extents (backend, conn); if (fl == -1) return -1; if (fl) diff --git a/server/protocol.c b/server/protocol.c index 72f6f2a8..72efcaa6 100644 --- a/server/protocol.c +++ b/server/protocol.c @@ -240,35 +240,27 @@ handle_request (struct connection *conn, switch (cmd) { case NBD_CMD_READ: - if (backend->pread (backend, conn, buf, count, offset, 0, &err) == -1) { - assert (err); + if (backend_pread (backend, conn, buf, count, offset, 0, &err) == -1) return err; - } break; case NBD_CMD_WRITE: if (fua) f |= NBDKIT_FLAG_FUA; - if (backend->pwrite (backend, conn, buf, count, offset, f, &err) == -1) { - assert (err); + if (backend_pwrite (backend, conn, buf, count, offset, f, &err) == -1) return err; - } break; case NBD_CMD_FLUSH: - if (backend->flush (backend, conn, 0, &err) == -1) { - assert (err); + if (backend_flush (backend, conn, 0, &err) == -1) return err; - } break; case NBD_CMD_TRIM: if (fua) f |= NBDKIT_FLAG_FUA; - if (backend->trim (backend, conn, count, offset, f, &err) == -1) { - assert (err); + if (backend_trim (backend, conn, count, offset, f, &err) == -1) return err; - } break; case NBD_CMD_CACHE: @@ -278,18 +270,14 @@ handle_request (struct connection *conn, while (count) { limit = MIN (count, sizeof buf); - if (backend->pread (backend, conn, buf, limit, offset, flags, - &err) == -1) { - assert (err); + if (backend_pread (backend, conn, buf, limit, offset, flags, + &err) == -1) return err; - } count -= limit; } } - else if (backend->cache (backend, conn, count, offset, 0, &err) == -1) { - assert (err); + else if (backend_cache (backend, conn, count, offset, 0, &err) == -1) return err; - } break; case NBD_CMD_WRITE_ZEROES: @@ -297,10 +285,8 @@ handle_request (struct connection *conn, f |= NBDKIT_FLAG_MAY_TRIM; if (fua) f |= NBDKIT_FLAG_FUA; - if (backend->zero (backend, conn, count, offset, f, &err) == -1) { - assert (err && err != ENOTSUP && err != EOPNOTSUPP); + if (backend_zero (backend, conn, count, offset, f, &err) == -1) return err; - } break; case NBD_CMD_BLOCK_STATUS: @@ -312,11 +298,9 @@ handle_request (struct connection *conn, if (conn->can_extents) { if (flags & NBD_CMD_FLAG_REQ_ONE) f |= NBDKIT_FLAG_REQ_ONE; - if (backend->extents (backend, conn, count, offset, f, - extents, &err) == -1) { - assert (err); + if (backend_extents (backend, conn, count, offset, f, + extents, &err) == -1) return err; - } } else { int r; diff --git a/tests/test-layers.c b/tests/test-layers.c index 6617cd73..a7184d72 100644 --- a/tests/test-layers.c +++ b/tests/test-layers.c @@ -1,5 +1,5 @@ /* nbdkit - * Copyright (C) 2018 Red Hat Inc. + * Copyright (C) 2018-2019 Red Hat Inc. * * Redistribution and use in source and binary forms, with or without * modification, are permitted provided that the following conditions are @@ -400,13 +400,13 @@ main (int argc, char *argv[]) sleep (1); log_verify_seen_in_order - ("testlayersfilter3: pread count=512 offset=0 flags=0x0", + ("testlayersfilter3: pread count=512 offset=0", "filter3: test_layers_filter_pread", - "testlayersfilter2: pread count=512 offset=0 flags=0x0", + "testlayersfilter2: pread count=512 offset=0", "filter2: test_layers_filter_pread", - "testlayersfilter1: pread count=512 offset=0 flags=0x0", + "testlayersfilter1: pread count=512 offset=0", "filter1: test_layers_filter_pread", - "testlayersplugin: debug: pread count=512 offset=0", + "testlayersplugin: pread count=512 offset=0", "test_layers_plugin_pread", NULL); @@ -434,13 +434,13 @@ main (int argc, char *argv[]) sleep (1); log_verify_seen_in_order - ("testlayersfilter3: pwrite count=512 offset=0 flags=0x0", + ("testlayersfilter3: pwrite count=512 offset=0 fua=0", "filter3: test_layers_filter_pwrite", - "testlayersfilter2: pwrite count=512 offset=0 flags=0x0", + "testlayersfilter2: pwrite count=512 offset=0 fua=0", "filter2: test_layers_filter_pwrite", - "testlayersfilter1: pwrite count=512 offset=0 flags=0x0", + "testlayersfilter1: pwrite count=512 offset=0 fua=0", "filter1: test_layers_filter_pwrite", - "testlayersplugin: debug: pwrite count=512 offset=0", + "testlayersplugin: pwrite count=512 offset=0 fua=0", "test_layers_plugin_pwrite", NULL); @@ -464,13 +464,13 @@ main (int argc, char *argv[]) sleep (1); log_verify_seen_in_order - ("testlayersfilter3: flush flags=0x0", + ("testlayersfilter3: flush", "filter3: test_layers_filter_flush", - "testlayersfilter2: flush flags=0x0", + "testlayersfilter2: flush", "filter2: test_layers_filter_flush", - "testlayersfilter1: flush flags=0x0", + "testlayersfilter1: flush", "filter1: test_layers_filter_flush", - "testlayersplugin: debug: flush", + "testlayersplugin: flush", "test_layers_plugin_flush", NULL); @@ -494,13 +494,13 @@ main (int argc, char *argv[]) sleep (1); log_verify_seen_in_order - ("testlayersfilter3: trim count=512 offset=0 flags=0x0", + ("testlayersfilter3: trim count=512 offset=0 fua=0", "filter3: test_layers_filter_trim", - "testlayersfilter2: trim count=512 offset=0 flags=0x0", + "testlayersfilter2: trim count=512 offset=0 fua=0", "filter2: test_layers_filter_trim", - "testlayersfilter1: trim count=512 offset=0 flags=0x0", + "testlayersfilter1: trim count=512 offset=0 fua=0", "filter1: test_layers_filter_trim", - "testlayersplugin: debug: trim count=512 offset=0", + "testlayersplugin: trim count=512 offset=0 fua=0", "test_layers_plugin_trim", NULL); @@ -524,13 +524,13 @@ main (int argc, char *argv[]) sleep (1); log_verify_seen_in_order - ("testlayersfilter3: zero count=512 offset=0 flags=0x1", + ("testlayersfilter3: zero count=512 offset=0 may_trim=1 fua=0", "filter3: test_layers_filter_zero", - "testlayersfilter2: zero count=512 offset=0 flags=0x1", + "testlayersfilter2: zero count=512 offset=0 may_trim=1 fua=0", "filter2: test_layers_filter_zero", - "testlayersfilter1: zero count=512 offset=0 flags=0x1", + "testlayersfilter1: zero count=512 offset=0 may_trim=1 fua=0", "filter1: test_layers_filter_zero", - "testlayersplugin: debug: zero count=512 offset=0 may_trim=1 fua=0", + "testlayersplugin: zero count=512 offset=0 may_trim=1 fua=0", "test_layers_plugin_zero", NULL); @@ -554,13 +554,13 @@ main (int argc, char *argv[]) sleep (1); log_verify_seen_in_order - ("testlayersfilter3: cache count=512 offset=0 flags=0x0", + ("testlayersfilter3: cache count=512 offset=0", "filter3: test_layers_filter_cache", - "testlayersfilter2: cache count=512 offset=0 flags=0x0", + "testlayersfilter2: cache count=512 offset=0", "filter2: test_layers_filter_cache", - "testlayersfilter1: cache count=512 offset=0 flags=0x0", + "testlayersfilter1: cache count=512 offset=0", "filter1: test_layers_filter_cache", - "testlayersplugin: debug: cache count=512 offset=0", + "testlayersplugin: cache count=512 offset=0", "test_layers_plugin_cache", NULL); -- 2.21.0

We don't know how long a plugin's .get_size() will take, but we also documented that it shouldn't change per connection and therefore can be cached. It's not hard to see that we have to consult the size per connection (see commit b3a43ccd for a test that purposefully exposes different sizes to separate clients), nor to search the code to see we already cache it at the protocol level, given that each .extents call refers to the current size: $ cat script case "$1" in get_size) sleep 1; echo 1m;; can_extents) ;; extents) echo 0 1m;; *) exit 2 ;; esac $ /bin/time -f %e \ ./nbdkit -U - sh script --run 'qemu-io -r -f raw -c map -c map $nbd' 1 MiB (0x100000) bytes allocated at offset 0 bytes (0x0) 1 MiB (0x100000) bytes allocated at offset 0 bytes (0x0) 1.08 and we see completion in just over a second, so the sleep in get_size is called only once. But we are not caching it in all filters: $ /bin/time -f %e \ ./nbdkit -U - --filter=offset sh script offset=512k \ --run 'qemu-io -r -f raw -c map -c map $nbd' 512 KiB (0x80000) bytes allocated at offset 0 bytes (0x0) 512 KiB (0x80000) bytes allocated at offset 0 bytes (0x0) 3.13 where the 3 second delay demonstrates that the offset filter calls next->get_size() once per top-level .extents call. The previous patches made it easy to support a framework for per-backend caching, so now to put it to use by remembering the result of get_size() for each. With that, the above example with offset filtering speeds up to a completion in just over one second. Signed-off-by: Eric Blake <eblake(a)redhat.com&gt; --- docs/nbdkit-filter.pod | 9 +++++---- server/internal.h | 3 +-- server/backend.c | 7 +++++-- server/connections.c | 3 +++ server/protocol-handshake-newstyle.c | 18 ++++++++++-------- server/protocol-handshake-oldstyle.c | 1 - server/protocol.c | 5 +++-- 7 files changed, 27 insertions(+), 19 deletions(-) diff --git a/docs/nbdkit-filter.pod b/docs/nbdkit-filter.pod index cfd664eb..1e2fe99c 100644 --- a/docs/nbdkit-filter.pod +++ b/docs/nbdkit-filter.pod @@ -346,10 +346,11 @@ will see. The returned size must be E<ge> 0. If there is an error, C<.get_size> should call C<nbdkit_error> with an error message and return C<-1>. -If this function is called more than once for the same connection, it -should return the same value; similarly, the filter may cache -C<next_ops-E<gt>get_size> for a given connection rather than repeating -calls. +This function is only called once per connection and cached by nbdkit. +Similarly, repeated calls to C<next_ops-E<gt>get_size> will returnf a +cached value; by calling into the plugin during C<.prepare>, you can +ensure that later calls during data commands like <.pread> will not +fail. =head2 C<.can_write> diff --git a/server/internal.h b/server/internal.h index 9bf84022..ec8a894c 100644 --- a/server/internal.h +++ b/server/internal.h @@ -151,7 +151,7 @@ typedef void (*connection_close_function) (struct connection *) struct b_conn_handle { void *handle; - // TODO add per-backend caching + uint64_t exportsize; }; struct connection { @@ -168,7 +168,6 @@ struct connection { size_t nr_handles; uint32_t cflags; - uint64_t exportsize; uint16_t eflags; bool readonly; bool can_flush; diff --git a/server/backend.c b/server/backend.c index b2054aa2..374d8540 100644 --- a/server/backend.c +++ b/server/backend.c @@ -132,10 +132,13 @@ backend_set_handle (struct backend *b, struct connection *conn, void *handle) int64_t backend_get_size (struct backend *b, struct connection *conn) { + struct b_conn_handle *h = &conn->handles[b->i]; + debug ("%s: get_size", b->name); - /* TODO caching */ - return b->get_size (b, conn); + if (h->exportsize == -1) + h->exportsize = b->get_size (b, conn); + return h->exportsize; } int diff --git a/server/connections.c b/server/connections.c index 1c501002..2a5150cd 100644 --- a/server/connections.c +++ b/server/connections.c @@ -260,6 +260,7 @@ new_connection (int sockin, int sockout, int nworkers) struct connection *conn; int opt; socklen_t optlen = sizeof opt; + struct backend *b; conn = calloc (1, sizeof *conn); if (conn == NULL) { @@ -273,6 +274,8 @@ new_connection (int sockin, int sockout, int nworkers) return NULL; } conn->nr_handles = backend->i + 1; + for_each_backend (b) + conn->handles[b->i].exportsize = -1; conn->status = 1; conn->nworkers = nworkers; diff --git a/server/protocol-handshake-newstyle.c b/server/protocol-handshake-newstyle.c index 28817317..23579e5d 100644 --- a/server/protocol-handshake-newstyle.c +++ b/server/protocol-handshake-newstyle.c @@ -117,7 +117,7 @@ send_newstyle_option_reply_exportname (struct connection *conn, static int send_newstyle_option_reply_info_export (struct connection *conn, uint32_t option, uint32_t reply, - uint16_t info) + uint16_t info, uint64_t exportsize) { struct fixed_new_option_reply fixed_new_option_reply; struct fixed_new_option_reply_info_export export; @@ -127,7 +127,7 @@ send_newstyle_option_reply_info_export (struct connection *conn, fixed_new_option_reply.reply = htobe32 (reply); fixed_new_option_reply.replylen = htobe32 (sizeof export); export.info = htobe16 (info); - export.exportsize = htobe64 (conn->exportsize); + export.exportsize = htobe64 (exportsize); export.eflags = htobe16 (conn->eflags); if (conn->send (conn, @@ -201,7 +201,7 @@ conn_recv_full (struct connection *conn, void *buf, size_t len, * in that function. */ static int -finish_newstyle_options (struct connection *conn) +finish_newstyle_options (struct connection *conn, uint64_t *exportsize) { int64_t r; @@ -213,7 +213,7 @@ finish_newstyle_options (struct connection *conn) "(%" PRIi64 ")", r); return -1; } - conn->exportsize = (uint64_t) r; + *exportsize = r; if (protocol_compute_eflags (conn, &conn->eflags) < 0) return -1; @@ -233,6 +233,7 @@ negotiate_handshake_newstyle_options (struct connection *conn) char data[MAX_OPTION_LENGTH+1]; struct new_handshake_finish handshake_finish; const char *optname; + uint64_t exportsize; for (nr_options = 0; nr_options < MAX_NR_OPTIONS; ++nr_options) { if (conn_recv_full (conn, &new_option, sizeof new_option, @@ -281,11 +282,11 @@ negotiate_handshake_newstyle_options (struct connection *conn) name_of_nbd_opt (option), data); /* We have to finish the handshake by sending handshake_finish. */ - if (finish_newstyle_options (conn) == -1) + if (finish_newstyle_options (conn, &exportsize) == -1) return -1; memset (&handshake_finish, 0, sizeof handshake_finish); - handshake_finish.exportsize = htobe64 (conn->exportsize); + handshake_finish.exportsize = htobe64 (exportsize); handshake_finish.eflags = htobe16 (conn->eflags); if (conn->send (conn, @@ -432,12 +433,13 @@ negotiate_handshake_newstyle_options (struct connection *conn) * qemu client in particular does not request this, but will * fail if we don't send it. */ - if (finish_newstyle_options (conn) == -1) + if (finish_newstyle_options (conn, &exportsize) == -1) return -1; if (send_newstyle_option_reply_info_export (conn, option, NBD_REP_INFO, - NBD_INFO_EXPORT) == -1) + NBD_INFO_EXPORT, + exportsize) == -1) return -1; /* For now we ignore all other info requests (but we must diff --git a/server/protocol-handshake-oldstyle.c b/server/protocol-handshake-oldstyle.c index af3ac488..3d44e9db 100644 --- a/server/protocol-handshake-oldstyle.c +++ b/server/protocol-handshake-oldstyle.c @@ -68,7 +68,6 @@ protocol_handshake_oldstyle (struct connection *conn) return -1; } exportsize = (uint64_t) r; - conn->exportsize = exportsize; gflags = 0; if (protocol_compute_eflags (conn, &eflags) < 0) diff --git a/server/protocol.c b/server/protocol.c index 72efcaa6..06f1ee15 100644 --- a/server/protocol.c +++ b/server/protocol.c @@ -53,8 +53,9 @@ static bool valid_range (struct connection *conn, uint64_t offset, uint32_t count) { - uint64_t exportsize = conn->exportsize; + uint64_t exportsize = backend_get_size (backend, conn); + assert (exportsize <= INT64_MAX); /* Guaranteed by negotiation phase */ return count > 0 && offset <= exportsize && offset + count <= exportsize; } @@ -705,7 +706,7 @@ protocol_recv_request_send_reply (struct connection *conn) /* Allocate the extents list for block status only. */ if (cmd == NBD_CMD_BLOCK_STATUS) { - extents = nbdkit_extents_new (offset, conn->exportsize); + extents = nbdkit_extents_new (offset, backend_get_size (backend, conn)); if (extents == NULL) { error = ENOMEM; goto send_reply; -- 2.21.0

Similar to the previous patch in caching size, we want to avoid calling into the plugin more than once per connection on any of the flag determination callbacks. The following script demonstrates the speedup, where we avoid repeated calls into a slow can_fua. Pre-patch: $ cat script case "$1" in get_size) echo 1m;; can_fua) sleep 1; echo native;; can_write | can_zero | pwrite | zero) ;; *) exit 2 ;; esac $ /bin/time -f %e ./nbdkit --filter=blocksize sh script maxlen=4k \ --run 'qemu-io -f raw -c "w -z -f 0 16k" $nbd' wrote 16384/16384 bytes at offset 0 16 KiB, 1 ops; 0:00:05.07 (3.157 KiB/sec and 0.1973 ops/sec) 6.14 there are six executions (one to determine eflags, one by the blocksize filter to determine whether to pass the flag down or call flush, and four by each .zero call over the split range from blocksize). After: $ /bin/time -f %e ./nbdkit --filter=blocksize sh script maxlen=4k \ --run 'qemu-io -f raw -c "w -z -f 0 16k" $nbd' wrote 16384/16384 bytes at offset 0 16 KiB, 1 ops; 00.03 sec (585.570 KiB/sec and 36.5981 ops/sec) 1.13 we've reduced things to a single call, and everything else using the cache. Note that the cache of can_zero answers the filter semantics (whether we advertise zero support in eflags) and not the plugin semantics (whether we should attempt .zero or just automatically fall back to .pwrite); that will be improved in the next patch. In the cow filter, adding a call to next_ops->can_cache in .prepare means that .cache no longer has to worry about can_cache failing (the can_FOO methods do not guarantee easy access to a sane errno value). Signed-off-by: Eric Blake <eblake(a)redhat.com&gt; --- docs/nbdkit-filter.pod | 9 +-- docs/nbdkit-plugin.pod | 6 +- server/internal.h | 19 +++--- server/backend.c | 75 ++++++++++++++++++++--- server/connections.c | 3 +- server/plugins.c | 7 ++- server/protocol-handshake.c | 87 +++++++++++---------------- server/protocol.c | 110 +++++++++++++++++++++------------- filters/blocksize/blocksize.c | 2 - filters/cache/cache.c | 1 - filters/cow/cow.c | 16 +++-- 11 files changed, 204 insertions(+), 131 deletions(-) diff --git a/docs/nbdkit-filter.pod b/docs/nbdkit-filter.pod index 1e2fe99c..3333d6b5 100644 --- a/docs/nbdkit-filter.pod +++ b/docs/nbdkit-filter.pod @@ -415,10 +415,11 @@ on to only the final sub-request, or by dropping the flag and ending with a direct call to C<next_ops-E<gt>flush>). If there is an error, the callback should call C<nbdkit_error> with an -error message and return C<-1>. If these functions are called more -than once for the same connection, they should return the same value; -similarly, the filter may cache the results of each counterpart in -C<next_ops> for a given connection rather than repeating calls. +error message and return C<-1>. These functions are only called once +per connection and cached by nbdkit. Similarly, repeated calls to any +of the C<next_ops> counterparts will return a cached value; by calling +into the plugin during C<.prepare>, you can ensure that later use of +the cached values during data commands like <.pwrite> will not fail. =head2 C<.pread> diff --git a/docs/nbdkit-plugin.pod b/docs/nbdkit-plugin.pod index bc3d9749..17239a5c 100644 --- a/docs/nbdkit-plugin.pod +++ b/docs/nbdkit-plugin.pod @@ -150,10 +150,8 @@ A new client has connected. These are called during option negotiation with the client, but before any data is served. These callbacks may return different values -across different C<.open> calls, but within a single connection, must -always return the same value; other code in nbdkit may cache the -per-connection value returned rather than using the callback a second -time. +across different C<.open> calls, but within a single connection, they +are called at most once and cached by nbdkit for that connection. =item C<.pread>, C<.pwrite> and other data serving callbacks diff --git a/server/internal.h b/server/internal.h index ec8a894c..ddb79623 100644 --- a/server/internal.h +++ b/server/internal.h @@ -152,6 +152,15 @@ struct b_conn_handle { void *handle; uint64_t exportsize; + int can_write; + int can_flush; + int is_rotational; + int can_trim; + int can_zero; + int can_fua; + int can_multi_conn; + int can_cache; + int can_extents; }; struct connection { @@ -169,16 +178,6 @@ struct connection { uint32_t cflags; uint16_t eflags; - bool readonly; - bool can_flush; - bool is_rotational; - bool can_trim; - bool can_zero; - bool can_fua; - bool can_multi_conn; - bool can_cache; - bool emulate_cache; - bool can_extents; bool using_tls; bool structured_replies; bool meta_context_base_allocation; diff --git a/server/backend.c b/server/backend.c index 374d8540..196b48e4 100644 --- a/server/backend.c +++ b/server/backend.c @@ -144,73 +144,130 @@ backend_get_size (struct backend *b, struct connection *conn) int backend_can_write (struct backend *b, struct connection *conn) { + struct b_conn_handle *h = &conn->handles[b->i]; + debug ("%s: can_write", b->name); - return b->can_write (b, conn); + if (h->can_write == -1) + h->can_write = b->can_write (b, conn); + return h->can_write; } int backend_can_flush (struct backend *b, struct connection *conn) { + struct b_conn_handle *h = &conn->handles[b->i]; + debug ("%s: can_flush", b->name); - return b->can_flush (b, conn); + if (h->can_flush == -1) + h->can_flush = b->can_flush (b, conn); + return h->can_flush; } int backend_is_rotational (struct backend *b, struct connection *conn) { + struct b_conn_handle *h = &conn->handles[b->i]; + debug ("%s: is_rotational", b->name); - return b->is_rotational (b, conn); + if (h->is_rotational == -1) + h->is_rotational = b->is_rotational (b, conn); + return h->is_rotational; } int backend_can_trim (struct backend *b, struct connection *conn) { + struct b_conn_handle *h = &conn->handles[b->i]; + int r; + debug ("%s: can_trim", b->name); - return b->can_trim (b, conn); + if (h->can_trim == -1) { + r = backend_can_write (b, conn); + if (r != 1) { + h->can_trim = 0; + return r; + } + h->can_trim = b->can_trim (b, conn); + } + return h->can_trim; } int backend_can_zero (struct backend *b, struct connection *conn) { + struct b_conn_handle *h = &conn->handles[b->i]; + int r; + debug ("%s: can_zero", b->name); - return b->can_zero (b, conn); + if (h->can_zero == -1) { + r = backend_can_write (b, conn); + if (r != 1) { + h->can_zero = 0; + return r; + } + h->can_zero = b->can_zero (b, conn); + } + return h->can_zero; } int backend_can_extents (struct backend *b, struct connection *conn) { + struct b_conn_handle *h = &conn->handles[b->i]; + debug ("%s: can_extents", b->name); - return b->can_extents (b, conn); + if (h->can_extents == -1) + h->can_extents = b->can_extents (b, conn); + return h->can_extents; } int backend_can_fua (struct backend *b, struct connection *conn) { + struct b_conn_handle *h = &conn->handles[b->i]; + int r; + debug ("%s: can_fua", b->name); - return b->can_fua (b, conn); + if (h->can_fua == -1) { + r = backend_can_write (b, conn); + if (r != 1) { + h->can_fua = NBDKIT_FUA_NONE; + return r; + } + h->can_fua = b->can_fua (b, conn); + } + return h->can_fua; } int backend_can_multi_conn (struct backend *b, struct connection *conn) { + struct b_conn_handle *h = &conn->handles[b->i]; + debug ("%s: can_multi_conn", b->name); - return b->can_multi_conn (b, conn); + if (h->can_multi_conn == -1) + h->can_multi_conn = b->can_multi_conn (b, conn); + return h->can_multi_conn; } int backend_can_cache (struct backend *b, struct connection *conn) { + struct b_conn_handle *h = &conn->handles[b->i]; + debug ("%s: can_cache", b->name); - return b->can_cache (b, conn); + if (h->can_cache == -1) + h->can_cache = b->can_cache (b, conn); + return h->can_cache; } int diff --git a/server/connections.c b/server/connections.c index 2a5150cd..7609e9a7 100644 --- a/server/connections.c +++ b/server/connections.c @@ -274,8 +274,9 @@ new_connection (int sockin, int sockout, int nworkers) return NULL; } conn->nr_handles = backend->i + 1; + memset (conn->handles, -1, conn->nr_handles * sizeof *conn->handles); for_each_backend (b) - conn->handles[b->i].exportsize = -1; + conn->handles[b->i].handle = NULL; conn->status = 1; conn->nworkers = nworkers; diff --git a/server/plugins.c b/server/plugins.c index d1654f8d..c8f4af90 100644 --- a/server/plugins.c +++ b/server/plugins.c @@ -509,7 +509,7 @@ plugin_pwrite (struct backend *b, struct connection *conn, assert (connection_get_handle (conn, 0)); - if (fua && plugin_can_fua (b, conn) != NBDKIT_FUA_NATIVE) { + if (fua && backend_can_fua (b, conn) != NBDKIT_FUA_NATIVE) { flags &= ~NBDKIT_FLAG_FUA; need_flush = true; } @@ -541,7 +541,7 @@ plugin_trim (struct backend *b, struct connection *conn, assert (connection_get_handle (conn, 0)); - if (fua && plugin_can_fua (b, conn) != NBDKIT_FUA_NATIVE) { + if (fua && backend_can_fua (b, conn) != NBDKIT_FUA_NATIVE) { flags &= ~NBDKIT_FLAG_FUA; need_flush = true; } @@ -574,13 +574,14 @@ plugin_zero (struct backend *b, struct connection *conn, assert (connection_get_handle (conn, 0)); - if (fua && plugin_can_fua (b, conn) != NBDKIT_FUA_NATIVE) { + if (fua && backend_can_fua (b, conn) != NBDKIT_FUA_NATIVE) { flags &= ~NBDKIT_FLAG_FUA; need_flush = true; } if (!count) return 0; if (p->plugin.can_zero) { + /* Calling backend_can_zero would answer the wrong question */ can_zero = p->plugin.can_zero (connection_get_handle (conn, 0)); assert (can_zero != -1); } diff --git a/server/protocol-handshake.c b/server/protocol-handshake.c index 4d12b3dc..16261c34 100644 --- a/server/protocol-handshake.c +++ b/server/protocol-handshake.c @@ -51,87 +51,72 @@ protocol_compute_eflags (struct connection *conn, uint16_t *flags) { uint16_t eflags = NBD_FLAG_HAS_FLAGS; int fl; + bool can_write = true; fl = backend_can_write (backend, conn); if (fl == -1) return -1; if (readonly || !fl) { eflags |= NBD_FLAG_READ_ONLY; - conn->readonly = true; + can_write = false; } - if (!conn->readonly) { - fl = backend_can_zero (backend, conn); - if (fl == -1) - return -1; - if (fl) { - eflags |= NBD_FLAG_SEND_WRITE_ZEROES; - conn->can_zero = true; - } - fl = backend_can_trim (backend, conn); - if (fl == -1) - return -1; - if (fl) { - eflags |= NBD_FLAG_SEND_TRIM; - conn->can_trim = true; - } + /* Check all flags even if they won't be advertised, to prime the + * cache and thus simplify later EINVAL handling of a client that + * makes a non-compliant request that did not match eflags. + */ + fl = backend_can_zero (backend, conn); + if (fl == -1) + return -1; + if (fl && can_write) + eflags |= NBD_FLAG_SEND_WRITE_ZEROES; - fl = backend_can_fua (backend, conn); - if (fl == -1) - return -1; - if (fl) { - eflags |= NBD_FLAG_SEND_FUA; - conn->can_fua = true; - } - } + fl = backend_can_trim (backend, conn); + if (fl == -1) + return -1; + if (fl && can_write) + eflags |= NBD_FLAG_SEND_TRIM; + + fl = backend_can_fua (backend, conn); + if (fl == -1) + return -1; + if (fl && can_write) + eflags |= NBD_FLAG_SEND_FUA; fl = backend_can_flush (backend, conn); if (fl == -1) return -1; - if (fl) { + if (fl) eflags |= NBD_FLAG_SEND_FLUSH; - conn->can_flush = true; - } fl = backend_is_rotational (backend, conn); if (fl == -1) return -1; - if (fl) { + if (fl) eflags |= NBD_FLAG_ROTATIONAL; - conn->is_rotational = true; - } - /* multi-conn is useless if parallel connections are not allowed */ - if (backend->thread_model (backend) > - NBDKIT_THREAD_MODEL_SERIALIZE_CONNECTIONS) { - fl = backend_can_multi_conn (backend, conn); - if (fl == -1) - return -1; - if (fl) { - eflags |= NBD_FLAG_CAN_MULTI_CONN; - conn->can_multi_conn = true; - } - } + /* multi-conn is useless if parallel connections are not allowed. */ + fl = backend_can_multi_conn (backend, conn); + if (fl == -1) + return -1; + if (fl && (backend->thread_model (backend) > + NBDKIT_THREAD_MODEL_SERIALIZE_CONNECTIONS)) + eflags |= NBD_FLAG_CAN_MULTI_CONN; fl = backend_can_cache (backend, conn); if (fl == -1) return -1; - if (fl) { + if (fl) eflags |= NBD_FLAG_SEND_CACHE; - conn->can_cache = true; - conn->emulate_cache = fl == NBDKIT_CACHE_EMULATE; - } - /* The result of this is not returned to callers here (or at any - * time during the handshake). However it makes sense to do it once - * per connection and store the result in the handle anyway. This - * protocol_compute_eflags function is a bit misnamed XXX. + /* The result of this is not directly advertised as part of the + * handshake, but priming the cache here makes BLOCK_STATUS handling + * not have to worry about errors, and makes test-layers easier to + * write. */ fl = backend_can_extents (backend, conn); if (fl == -1) return -1; - if (fl) - conn->can_extents = true; if (conn->structured_replies) eflags |= NBD_FLAG_SEND_DF; diff --git a/server/protocol.c b/server/protocol.c index 06f1ee15..0ecf0b5c 100644 --- a/server/protocol.c +++ b/server/protocol.c @@ -64,14 +64,19 @@ validate_request (struct connection *conn, uint16_t cmd, uint16_t flags, uint64_t offset, uint32_t count, uint32_t *error) { + int r; + /* Readonly connection? */ - if (conn->readonly && - (cmd == NBD_CMD_WRITE || cmd == NBD_CMD_TRIM || - cmd == NBD_CMD_WRITE_ZEROES)) { - nbdkit_error ("invalid request: %s: write request on readonly connection", - name_of_nbd_cmd (cmd)); - *error = EROFS; - return false; + if (cmd == NBD_CMD_WRITE || cmd == NBD_CMD_TRIM || + cmd == NBD_CMD_WRITE_ZEROES) { + r = backend_can_write (backend, conn); + assert (r >= 0); /* Guaranteed by eflags computation */ + if (!r) { + nbdkit_error ("invalid request: %s: write request on readonly connection", + name_of_nbd_cmd (cmd)); + *error = EROFS; + return false; + } } /* Validate cmd, offset, count. */ @@ -142,10 +147,14 @@ validate_request (struct connection *conn, *error = EINVAL; return false; } - if (!conn->can_fua && (flags & NBD_CMD_FLAG_FUA)) { - nbdkit_error ("invalid request: FUA flag not supported"); - *error = EINVAL; - return false; + if (flags & NBD_CMD_FLAG_FUA) { + r = backend_can_fua (backend, conn); + assert (r >= 0); /* Guaranteed by eflags computation */ + if (!r) { + nbdkit_error ("invalid request: FUA flag not supported"); + *error = EINVAL; + return false; + } } /* Refuse over-large read and write requests. */ @@ -159,35 +168,51 @@ validate_request (struct connection *conn, } /* Flush allowed? */ - if (!conn->can_flush && cmd == NBD_CMD_FLUSH) { - nbdkit_error ("invalid request: %s: flush operation not supported", - name_of_nbd_cmd (cmd)); - *error = EINVAL; - return false; + if (cmd == NBD_CMD_FLUSH) { + r = backend_can_flush (backend, conn); + assert (r >= 0); /* Guaranteed by eflags computation */ + if (!r) { + nbdkit_error ("invalid request: %s: flush operation not supported", + name_of_nbd_cmd (cmd)); + *error = EINVAL; + return false; + } } /* Trim allowed? */ - if (!conn->can_trim && cmd == NBD_CMD_TRIM) { - nbdkit_error ("invalid request: %s: trim operation not supported", - name_of_nbd_cmd (cmd)); - *error = EINVAL; - return false; + if (cmd == NBD_CMD_TRIM) { + r = backend_can_trim (backend, conn); + assert (r >= 0); /* Guaranteed by eflags computation */ + if (!r) { + nbdkit_error ("invalid request: %s: trim operation not supported", + name_of_nbd_cmd (cmd)); + *error = EINVAL; + return false; + } } /* Zero allowed? */ - if (!conn->can_zero && cmd == NBD_CMD_WRITE_ZEROES) { - nbdkit_error ("invalid request: %s: write zeroes operation not supported", - name_of_nbd_cmd (cmd)); - *error = EINVAL; - return false; + if (cmd == NBD_CMD_WRITE_ZEROES) { + r = backend_can_zero (backend, conn); + assert (r >= 0); /* Guaranteed by eflags computation */ + if (!r) { + nbdkit_error ("invalid request: %s: write zeroes operation not supported", + name_of_nbd_cmd (cmd)); + *error = EINVAL; + return false; + } } /* Cache allowed? */ - if (!conn->can_cache && cmd == NBD_CMD_CACHE) { - nbdkit_error ("invalid request: %s: cache operation not supported", - name_of_nbd_cmd (cmd)); - *error = EINVAL; - return false; + if (cmd == NBD_CMD_CACHE) { + r = backend_can_cache (backend, conn); + assert (r >= 0); /* Guaranteed by eflags computation */ + if (!r) { + nbdkit_error ("invalid request: %s: cache operation not supported", + name_of_nbd_cmd (cmd)); + *error = EINVAL; + return false; + } } /* Block status allowed? */ @@ -232,8 +257,8 @@ handle_request (struct connection *conn, void *buf, struct nbdkit_extents *extents) { uint32_t f = 0; - bool fua = conn->can_fua && (flags & NBD_CMD_FLAG_FUA); int err = 0; + int r; /* Clear the error, so that we know if the plugin calls * nbdkit_set_error() or relied on errno. */ @@ -246,7 +271,7 @@ handle_request (struct connection *conn, break; case NBD_CMD_WRITE: - if (fua) + if (flags & NBD_CMD_FLAG_FUA) f |= NBDKIT_FLAG_FUA; if (backend_pwrite (backend, conn, buf, count, offset, f, &err) == -1) return err; @@ -258,14 +283,16 @@ handle_request (struct connection *conn, break; case NBD_CMD_TRIM: - if (fua) + if (flags & NBD_CMD_FLAG_FUA) f |= NBDKIT_FLAG_FUA; if (backend_trim (backend, conn, count, offset, f, &err) == -1) return err; break; case NBD_CMD_CACHE: - if (conn->emulate_cache) { + r = backend_can_cache (backend, conn); + assert (r > 0); /* Guaranteed by validate_request */ + if (r == NBDKIT_CACHE_EMULATE) { static char buf[MAX_REQUEST_SIZE]; /* data sink, never read */ uint32_t limit; @@ -284,7 +311,7 @@ handle_request (struct connection *conn, case NBD_CMD_WRITE_ZEROES: if (!(flags & NBD_CMD_FLAG_NO_HOLE)) f |= NBDKIT_FLAG_MAY_TRIM; - if (fua) + if (flags & NBD_CMD_FLAG_FUA) f |= NBDKIT_FLAG_FUA; if (backend_zero (backend, conn, count, offset, f, &err) == -1) return err; @@ -293,10 +320,13 @@ handle_request (struct connection *conn, case NBD_CMD_BLOCK_STATUS: /* The other backend methods don't check can_*. That is because * those methods are implicitly suppressed by returning eflags to - * the client. However there is no eflag for extents so we must - * check it here. + * the client (see validate_request), but there is no eflag for + * extents. We did prime the cache earlier, but must check here + * in order to perform a fallback when needed. */ - if (conn->can_extents) { + r = backend_can_extents (backend, conn); + assert (r >= 0); /* Guaranteed during eflags computation */ + if (r) { if (flags & NBD_CMD_FLAG_REQ_ONE) f |= NBDKIT_FLAG_REQ_ONE; if (backend_extents (backend, conn, count, offset, f, @@ -304,8 +334,6 @@ handle_request (struct connection *conn, return err; } else { - int r; - /* By default it is safe assume that everything in the range is * allocated. */ diff --git a/filters/blocksize/blocksize.c b/filters/blocksize/blocksize.c index 0978887f..0fa05301 100644 --- a/filters/blocksize/blocksize.c +++ b/filters/blocksize/blocksize.c @@ -138,8 +138,6 @@ blocksize_config_complete (nbdkit_next_config_complete *next, void *nxdata) "maxdata=<SIZE> Maximum size for read/write (default 64M).\n" \ "maxlen=<SIZE> Maximum size for trim/zero (default 4G-minblock)." -/* TODO: Should we have a .prepare to cache per-connection FUA mode? */ - /* Round size down to avoid issues at end of file. */ static int64_t blocksize_get_size (struct nbdkit_next_ops *next_ops, void *nxdata, diff --git a/filters/cache/cache.c b/filters/cache/cache.c index b5dbccd2..e5f18d9b 100644 --- a/filters/cache/cache.c +++ b/filters/cache/cache.c @@ -239,7 +239,6 @@ cache_prepare (struct nbdkit_next_ops *next_ops, void *nxdata, r = cache_get_size (next_ops, nxdata, handle); if (r < 0) return -1; - /* TODO: cache per-connection FUA mode? */ return 0; } diff --git a/filters/cow/cow.c b/filters/cow/cow.c index 9d91d432..e4330bf3 100644 --- a/filters/cow/cow.c +++ b/filters/cow/cow.c @@ -127,7 +127,7 @@ cow_get_size (struct nbdkit_next_ops *next_ops, void *nxdata, return size; } -/* Force an early call to cow_get_size, consequently truncating the +/* Force early calls to populate nbdkit's cache, and truncate the * overlay to the correct size. */ static int @@ -137,7 +137,14 @@ cow_prepare (struct nbdkit_next_ops *next_ops, void *nxdata, int64_t r; r = cow_get_size (next_ops, nxdata, handle); - return r >= 0 ? 0 : -1; + if (r == -1) + return -1; + + r = next_ops->can_cache (nxdata); + if (r == -1) + return -1; + + return 0; } /* Whatever the underlying plugin can or can't do, we can write, we @@ -427,7 +434,7 @@ cow_cache (struct nbdkit_next_ops *next_ops, void *nxdata, uint64_t blknum, blkoffs; int r; uint64_t remaining = count; /* Rounding out could exceed 32 bits */ - enum cache_mode mode; /* XXX Cache this per connection? */ + enum cache_mode mode; switch (next_ops->can_cache (nxdata)) { case NBDKIT_CACHE_NONE: @@ -440,8 +447,7 @@ cow_cache (struct nbdkit_next_ops *next_ops, void *nxdata, mode = BLK_CACHE_PASSTHROUGH; break; default: - *err = EINVAL; - return -1; + abort (); /* .prepare populated the cache */ } if (cow_on_cache) mode = BLK_CACHE_COW; -- 2.21.0

On Thu, Aug 29, 2019 at 10:08:26PM -0500, Eric Blake wrote: So don't we need to keep the backend_can_fua() test here and later in this function? 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

The tri-state return pattern has proved valuable in other cases where a backend wants to opt in or out of nbdkit fallbacks. Using a tri-state return pattern at the backend level unifies the different semantics between plugin and filter .can_zero return values, and makes it possible for plugins to use cached results of .can_zero rather than calling into the plugin on each .zero request. As in other recent patches, it is easy to write an sh script that demonstrates a resulting speedup from the caching. All filters are in-tree and do not have a promise of API compatability, so it's easy to update all of them (the log filter is okay as-is, the nozero filter is easy to update, and no other filter overrides .can_zero). But for backwards compatibility reasons, we cannot change the semantics of the plugin .can_zero return value while remaining at API version 2; so that remains a bool, but now selects between EMULATE or NATIVE at the backend level. Signed-off-by: Eric Blake <eblake(a)redhat.com&gt; --- docs/nbdkit-filter.pod | 44 ++++++++++++++++++++++++++++++----------- server/backend.c | 2 +- server/plugins.c | 33 ++++++++++++++++--------------- include/nbdkit-filter.h | 4 ++++ filters/nozero/nozero.c | 9 ++++++++- 5 files changed, 63 insertions(+), 29 deletions(-) diff --git a/docs/nbdkit-filter.pod b/docs/nbdkit-filter.pod index 3333d6b5..c83fcbfe 100644 --- a/docs/nbdkit-filter.pod +++ b/docs/nbdkit-filter.pod @@ -393,16 +393,37 @@ fail. These intercept the corresponding plugin methods, and control feature bits advertised to the client. -Of note, the C<.can_zero> callback in the filter controls whether the -server advertises zero support to the client, which is slightly -different semantics than the plugin; that is, -C<next_ops-E<gt>can_zero> always returns true for a plugin, even when -the plugin's own C<.can_zero> callback returned false, because nbdkit -implements a fallback to C<.pwrite> at the plugin layer. +Of note, the semantics of C<.can_zero> callback in the filter are +slightly different from the plugin, and must be one of three success +values visible only to filters: + +=over 4 + +=item C<NBDKIT_ZERO_NONE> + +Completely suppress advertisement of write zero support (this can only +be done from filters, not plugins). + +=item C<NBDKIT_ZERO_EMULATE> + +Inform nbdkit that write zeroes should immediately fall back to +C<.pwrite> emulation without trying C<.zero> (this value is returned +by C<next_ops-E<gt>can_zero> if the plugin returned false in its +C<.can_zero>). + +=item C<NBDKIT_ZERO_NATIVE> + +Inform nbdkit that write zeroes should attempt to use C<.zero>, +although it may still fall back to C<.pwrite> emulation for C<ENOTSUP> +or C<EOPNOTSUPP> failures (this value is returned by +C<next_ops-E<gt>can_zero> if the plugin returned true in its +C<.can_zero>). + +=back Remember that most of the feature check functions return merely a -boolean success value, while C<.can_fua> and C<.can_cache> have three -success values. +boolean success value, while C<.can_zero>, C<.can_fua> and +C<.can_cache> have three success values. The difference between C<.can_fua> values may affect choices made in the filter: when splitting a write request that requested FUA from the @@ -514,9 +535,10 @@ value to return to the client. This intercepts the plugin C<.zero> method and can be used to modify zero requests. -This function will not be called if C<.can_zero> returned false; in -turn, the filter should not call C<next_ops-E<gt>zero> if -C<next_ops-E<gt>can_zero> did not return true. +This function will not be called if C<.can_zero> returned +C<NBDKIT_ZERO_NONE>; in turn, the filter should not call +C<next_ops-E<gt>zero> if C<next_ops-E<gt>can_zero> returned +C<NBDKIT_ZERO_NONE>. On input, the parameter C<flags> may include C<NBDKIT_FLAG_MAY_TRIM> unconditionally, and C<NBDKIT_FLAG_FUA> based on the result of diff --git a/server/backend.c b/server/backend.c index 196b48e4..1e3a0109 100644 --- a/server/backend.c +++ b/server/backend.c @@ -207,7 +207,7 @@ backend_can_zero (struct backend *b, struct connection *conn) if (h->can_zero == -1) { r = backend_can_write (b, conn); if (r != 1) { - h->can_zero = 0; + h->can_zero = NBDKIT_ZERO_NONE; return r; } h->can_zero = b->can_zero (b, conn); diff --git a/server/plugins.c b/server/plugins.c index c8f4af90..bff4cd47 100644 --- a/server/plugins.c +++ b/server/plugins.c @@ -354,19 +354,26 @@ static int plugin_can_zero (struct backend *b, struct connection *conn) { struct backend_plugin *p = container_of (b, struct backend_plugin, backend); + int r; assert (connection_get_handle (conn, 0)); - /* Note the special case here: the plugin's .can_zero controls only - * whether we call .zero; while the backend expects .can_zero to - * return whether to advertise zero support. Since we ALWAYS know - * how to fall back to .pwrite in plugin_zero(), we ignore the - * difference between the plugin's true or false return, and only - * call it to catch a -1 failure during negotiation. */ - if (p->plugin.can_zero && - p->plugin.can_zero (connection_get_handle (conn, 0)) == -1) + /* Note the special case here: the plugin's .can_zero returns a bool + * which controls only whether we call .zero; while the backend + * expects .can_zero to return a tri-state on level of support. + */ + if (p->plugin.can_zero) { + r = p->plugin.can_zero (connection_get_handle (conn, 0)); + if (r == -1) + return -1; + return r ? NBDKIT_ZERO_NATIVE : NBDKIT_ZERO_EMULATE; + } + if (p->plugin.zero || p->plugin._zero_old) + return NBDKIT_ZERO_NATIVE; + r = backend_can_write (b, conn); + if (r == -1) return -1; - return plugin_can_write (b, conn); + return r ? NBDKIT_ZERO_EMULATE : NBDKIT_ZERO_NONE; } static int @@ -570,7 +577,6 @@ plugin_zero (struct backend *b, struct connection *conn, bool fua = flags & NBDKIT_FLAG_FUA; bool emulate = false; bool need_flush = false; - int can_zero = 1; /* TODO cache this per-connection? */ assert (connection_get_handle (conn, 0)); @@ -580,13 +586,8 @@ plugin_zero (struct backend *b, struct connection *conn, } if (!count) return 0; - if (p->plugin.can_zero) { - /* Calling backend_can_zero would answer the wrong question */ - can_zero = p->plugin.can_zero (connection_get_handle (conn, 0)); - assert (can_zero != -1); - } - if (can_zero) { + if (backend_can_zero (b, conn) == NBDKIT_ZERO_NATIVE) { errno = 0; if (p->plugin.zero) r = p->plugin.zero (connection_get_handle (conn, 0), count, offset, diff --git a/include/nbdkit-filter.h b/include/nbdkit-filter.h index 29d92755..6232e0e2 100644 --- a/include/nbdkit-filter.h +++ b/include/nbdkit-filter.h @@ -43,6 +43,10 @@ extern "C" { #endif +#define NBDKIT_ZERO_NONE 0 +#define NBDKIT_ZERO_EMULATE 1 +#define NBDKIT_ZERO_NATIVE 2 + struct nbdkit_extent { uint64_t offset; uint64_t length; diff --git a/filters/nozero/nozero.c b/filters/nozero/nozero.c index 964cce9f..d916657e 100644 --- a/filters/nozero/nozero.c +++ b/filters/nozero/nozero.c @@ -94,7 +94,14 @@ nozero_prepare (struct nbdkit_next_ops *next_ops, void *nxdata, void *handle) static int nozero_can_zero (struct nbdkit_next_ops *next_ops, void *nxdata, void *handle) { - return zeromode != NONE; + switch (zeromode) { + case NONE: + return NBDKIT_ZERO_NONE; + case EMULATE: + return NBDKIT_ZERO_EMULATE; + default: + return next_ops->can_zero (nxdata); + } } static int -- 2.21.0

Now instead of validating just the client's request, we are validating that all filters are passing valid requests on down. In protocol.c, we were able to assert that our computation of eflags populated all of the flags, and thus calls such as backend_can_write would not fail; however, with filters, keeping those assertions mean the burden is now on the filter to avoid calling into next_ops->FOO without first priming the cache of next_ops->can_FOO. An audit of existing filters did not find any more culprits. Signed-off-by: Eric Blake <eblake(a)redhat.com&gt; --- server/backend.c | 94 ++++++++++++++++++++++++++++++++ server/protocol.c | 134 ++++++++++------------------------------------ 2 files changed, 121 insertions(+), 107 deletions(-) diff --git a/server/backend.c b/server/backend.c index e785a824..cd678f96 100644 --- a/server/backend.c +++ b/server/backend.c @@ -124,6 +124,43 @@ backend_unload (struct backend *b, void (*unload) (void)) free (b->name); } +static bool +writes_blocked (struct backend *b, struct connection *conn, const char *cmd, + uint32_t flags) +{ + struct b_conn_handle *h = &conn->handles[b->i]; + + assert (h->can_write >= 0); + if (h->can_write == 0) { + nbdkit_error ("invalid request: %s: write request on readonly connection", + cmd); + return true; + } + if (flags & NBDKIT_FLAG_FUA) { + assert (h->can_fua >= 0); + if (h->can_fua == 0) { + nbdkit_error ("invalid request: %s: FUA flag not supported", cmd); + return true; + } + } + return false; +} + +static bool +invalid_range (struct backend *b, struct connection *conn, const char *cmd, + uint64_t offset, uint32_t count) +{ + struct b_conn_handle *h = &conn->handles[b->i]; + + assert (h->exportsize >= 0); + if (!count || offset > h->exportsize || offset + count > h->exportsize) { + nbdkit_error ("invalid request: %s: offset and count are out of range: " + "offset=%" PRIu64 " count=%" PRIu32, cmd, offset, count); + return true; + } + return false; +} + void backend_set_handle (struct backend *b, struct connection *conn, void *handle) { @@ -283,6 +320,10 @@ backend_pread (struct backend *b, struct connection *conn, debug ("%s: pread count=%" PRIu32 " offset=%" PRIu64, b->name, count, offset); + if (invalid_range (b, conn, "pread", offset, count)) { + *err = EINVAL; + return -1; + } r = b->pread (b, conn, buf, count, offset, flags, err); if (r == -1) assert (*err); @@ -300,6 +341,14 @@ backend_pwrite (struct backend *b, struct connection *conn, debug ("%s: pwrite count=%" PRIu32 " offset=%" PRIu64 " fua=%d", b->name, count, offset, !!(flags & NBDKIT_FLAG_FUA)); + if (writes_blocked (b, conn, "pwrite", flags)) { + *err = EROFS; + return -1; + } + if (invalid_range (b, conn, "pwrite", offset, count)) { + *err = ENOSPC; + return -1; + } r = b->pwrite (b, conn, buf, count, offset, flags, err); if (r == -1) assert (*err); @@ -310,11 +359,18 @@ int backend_flush (struct backend *b, struct connection *conn, uint32_t flags, int *err) { + struct b_conn_handle *h = &conn->handles[b->i]; int r; assert (flags == 0); debug ("%s: flush", b->name); + assert (h->can_flush >= 0); + if (h->can_flush == 0) { + nbdkit_error ("invalid request: flush operation not supported"); + *err = EINVAL; + return -1; + } r = b->flush (b, conn, flags, err); if (r == -1) assert (*err); @@ -326,12 +382,27 @@ backend_trim (struct backend *b, struct connection *conn, uint32_t count, uint64_t offset, uint32_t flags, int *err) { + struct b_conn_handle *h = &conn->handles[b->i]; int r; assert (flags == 0); debug ("%s: trim count=%" PRIu32 " offset=%" PRIu64 " fua=%d", b->name, count, offset, !!(flags & NBDKIT_FLAG_FUA)); + if (writes_blocked (b, conn, "trim", flags)) { + *err = EROFS; + return -1; + } + if (invalid_range (b, conn, "trim", offset, count)) { + *err = EINVAL; + return -1; + } + assert (h->can_trim >= 0); + if (h->can_trim == 0) { + nbdkit_error ("invalid request: trim operation not supported"); + *err = EINVAL; + return -1; + } r = b->trim (b, conn, count, offset, flags, err); if (r == -1) assert (*err); @@ -343,6 +414,7 @@ backend_zero (struct backend *b, struct connection *conn, uint32_t count, uint64_t offset, uint32_t flags, int *err) { + struct b_conn_handle *h = &conn->handles[b->i]; int r; assert (!(flags & ~(NBDKIT_FLAG_MAY_TRIM | NBDKIT_FLAG_FUA))); @@ -350,6 +422,20 @@ backend_zero (struct backend *b, struct connection *conn, b->name, count, offset, !!(flags & NBDKIT_FLAG_MAY_TRIM), !!(flags & NBDKIT_FLAG_FUA)); + if (writes_blocked (b, conn, "zero", flags)) { + *err = EROFS; + return -1; + } + if (invalid_range (b, conn, "zero", offset, count)) { + *err = ENOSPC; + return -1; + } + assert (h->can_zero >= 0); + if (h->can_zero == NBDKIT_ZERO_NONE) { + nbdkit_error ("invalid request: write zeroes operation not supported"); + *err = EINVAL; + return -1; + } r = b->zero (b, conn, count, offset, flags, err); if (r == -1) assert (*err && *err != ENOTSUP && *err != EOPNOTSUPP); @@ -368,6 +454,10 @@ backend_extents (struct backend *b, struct connection *conn, debug ("%s: extents count=%" PRIu32 " offset=%" PRIu64 " req_one=%d", b->name, count, offset, !!(flags & NBDKIT_FLAG_REQ_ONE)); + if (invalid_range (b, conn, "extents", offset, count)) { + *err = EINVAL; + return -1; + } assert (h->can_extents >= 0); if (h->can_extents == 0) { /* By default it is safe assume that everything in the range is @@ -396,6 +486,10 @@ backend_cache (struct backend *b, struct connection *conn, debug ("%s: cache count=%" PRIu32 " offset=%" PRIu64, b->name, count, offset); + if (invalid_range (b, conn, "cache", offset, count)) { + *err = EINVAL; + return -1; + } assert (h->can_cache >= 0); if (h->can_cache == NBDKIT_CACHE_NONE) { nbdkit_error ("invalid request: cache operation not supported"); diff --git a/server/protocol.c b/server/protocol.c index 03862f5f..db40cca3 100644 --- a/server/protocol.c +++ b/server/protocol.c @@ -47,50 +47,43 @@ #include "minmax.h" #include "protocol.h" -static bool -valid_range (struct connection *conn, uint64_t offset, uint32_t count) -{ - uint64_t exportsize = backend_get_size (backend, conn); - - assert (exportsize <= INT64_MAX); /* Guaranteed by negotiation phase */ - return count > 0 && offset <= exportsize && offset + count <= exportsize; -} - static bool validate_request (struct connection *conn, uint16_t cmd, uint16_t flags, uint64_t offset, uint32_t count, uint32_t *error) { - int r; - - /* Readonly connection? */ - if (cmd == NBD_CMD_WRITE || cmd == NBD_CMD_TRIM || - cmd == NBD_CMD_WRITE_ZEROES) { - r = backend_can_write (backend, conn); - assert (r >= 0); /* Guaranteed by eflags computation */ - if (!r) { - nbdkit_error ("invalid request: %s: write request on readonly connection", - name_of_nbd_cmd (cmd)); - *error = EROFS; - return false; - } - } - - /* Validate cmd, offset, count. */ + /* Validate command arguments (more checks will be done later in backend) */ switch (cmd) { - case NBD_CMD_READ: case NBD_CMD_CACHE: - case NBD_CMD_WRITE: case NBD_CMD_TRIM: case NBD_CMD_WRITE_ZEROES: + break; + + case NBD_CMD_READ: + case NBD_CMD_WRITE: + /* Refuse over-large read and write requests. */ + if (count > MAX_REQUEST_SIZE) { + nbdkit_error ("invalid request: %s: data request is too large (%" PRIu32 + " > %d)", + name_of_nbd_cmd (cmd), count, MAX_REQUEST_SIZE); + *error = ENOMEM; + return false; + } + break; + case NBD_CMD_BLOCK_STATUS: - if (!valid_range (conn, 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, - name_of_nbd_cmd (cmd), offset, count); - *error = (cmd == NBD_CMD_WRITE || - cmd == NBD_CMD_WRITE_ZEROES) ? ENOSPC : EINVAL; + if (!conn->structured_replies) { + nbdkit_error ("invalid request: " + "%s: structured replies was not negotiated", + name_of_nbd_cmd (cmd)); + *error = EINVAL; + return false; + } + if (!conn->meta_context_base_allocation) { + nbdkit_error ("invalid request: " + "%s: base:allocation was not negotiated", + name_of_nbd_cmd (cmd)); + *error = EINVAL; return false; } break; @@ -144,79 +137,6 @@ validate_request (struct connection *conn, *error = EINVAL; return false; } - if (flags & NBD_CMD_FLAG_FUA) { - r = backend_can_fua (backend, conn); - assert (r >= 0); /* Guaranteed by eflags computation */ - if (!r) { - nbdkit_error ("invalid request: FUA flag not supported"); - *error = EINVAL; - return false; - } - } - - /* Refuse over-large read and write requests. */ - if ((cmd == NBD_CMD_WRITE || cmd == NBD_CMD_READ) && - count > MAX_REQUEST_SIZE) { - nbdkit_error ("invalid request: %s: data request is too large (%" PRIu32 - " > %d)", - name_of_nbd_cmd (cmd), count, MAX_REQUEST_SIZE); - *error = ENOMEM; - return false; - } - - /* Flush allowed? */ - if (cmd == NBD_CMD_FLUSH) { - r = backend_can_flush (backend, conn); - assert (r >= 0); /* Guaranteed by eflags computation */ - if (!r) { - nbdkit_error ("invalid request: %s: flush operation not supported", - name_of_nbd_cmd (cmd)); - *error = EINVAL; - return false; - } - } - - /* Trim allowed? */ - if (cmd == NBD_CMD_TRIM) { - r = backend_can_trim (backend, conn); - assert (r >= 0); /* Guaranteed by eflags computation */ - if (!r) { - nbdkit_error ("invalid request: %s: trim operation not supported", - name_of_nbd_cmd (cmd)); - *error = EINVAL; - return false; - } - } - - /* Zero allowed? */ - if (cmd == NBD_CMD_WRITE_ZEROES) { - r = backend_can_zero (backend, conn); - assert (r >= 0); /* Guaranteed by eflags computation */ - if (!r) { - nbdkit_error ("invalid request: %s: write zeroes operation not supported", - name_of_nbd_cmd (cmd)); - *error = EINVAL; - return false; - } - } - - /* Block status allowed? */ - if (cmd == NBD_CMD_BLOCK_STATUS) { - if (!conn->structured_replies) { - nbdkit_error ("invalid request: " - "%s: structured replies was not negotiated", - name_of_nbd_cmd (cmd)); - *error = EINVAL; - return false; - } - if (!conn->meta_context_base_allocation) { - nbdkit_error ("invalid request: " - "%s: base:allocation was not negotiated", - name_of_nbd_cmd (cmd)); - *error = EINVAL; - return false; - } - } return true; /* Command validates. */ } -- 2.21.0