search for: lock_unload

...fter-free and uninitialized read problems when using glibc. This in fact reveals a race during filter shutdown (which this commit does not fix): Thread 2 (Thread 0x7f1caaa5ffc0 (LWP 7223)): #0 0x00007f1cab0a05f8 in pthread_rwlock_wrlock () from /lib64/libpthread.so.0 #1 0x0000000000408842 in lock_unload () at locks.c:97 #2 0x00000000004066ff in filter_free (b=0x203c330) at filters.c:77 #3 0x000000000040a6f4 in main (argc=11, argv=0x7ffc1f4486e8) at main.c:649 Thread 1 (Thread 0x7f1caaa5e700 (LWP 7226)): #0 0x000000000040732a in filter_finalize (b=0x203c330, conn=0x203d870) at filters....

...plugin_zero (struct connection *conn, uint32_t count, uint64_t offset, int may_trim); +/* locks.c */ +extern void lock_connection (void); +extern void unlock_connection (void); +extern void lock_request (struct connection *conn); +extern void unlock_request (struct connection *conn); +extern void lock_unload (void); +extern void unlock_unload (void); + /* sockets.c */ extern int *bind_unix_socket (size_t *); extern int *bind_tcpip_socket (size_t *); diff --git a/src/locks.c b/src/locks.c new file mode 100644 index 0000000..6021356 --- /dev/null +++ b/src/locks.c @@ -0,0 +1,115 @@ +/* nbdkit + * Copy...

...plugin_zero (struct connection *conn, uint32_t count, uint64_t offset, int may_trim); +/* locks.c */ +extern void lock_connection (void); +extern void unlock_connection (void); +extern void lock_request (struct connection *conn); +extern void unlock_request (struct connection *conn); +extern void lock_unload (void); +extern void unlock_unload (void); + /* sockets.c */ extern int *bind_unix_socket (size_t *); extern int *bind_tcpip_socket (size_t *); diff --git a/src/locks.c b/src/locks.c new file mode 100644 index 0000000..6021356 --- /dev/null +++ b/src/locks.c @@ -0,0 +1,115 @@ +/* nbdkit + * Copy...

...oblems when using glibc. > > This in fact reveals a race during filter shutdown (which this > commit does not fix): > > Thread 2 (Thread 0x7f1caaa5ffc0 (LWP 7223)): > #0 0x00007f1cab0a05f8 in pthread_rwlock_wrlock () from /lib64/libpthread.so.0 > #1 0x0000000000408842 in lock_unload () at locks.c:97 > #2 0x00000000004066ff in filter_free (b=0x203c330) at filters.c:77 Do we need some sort of pthread_join() in filter_free() to allow... > #3 0x000000000040a6f4 in main (argc=11, argv=0x7ffc1f4486e8) at main.c:649 > > Thread 1 (Thread 0x7f1caaa5e700 (LWP 7226)):...

...) + abort (); - if (thread_model <= NBDKIT_THREAD_MODEL_SERIALIZE_ALL_REQUESTS) - pthread_mutex_unlock (&all_requests_lock); + if (thread_model <= NBDKIT_THREAD_MODEL_SERIALIZE_ALL_REQUESTS && + pthread_mutex_unlock (&all_requests_lock)) + abort (); } void lock_unload (void) { - pthread_rwlock_wrlock (&unload_prevention_lock); + if (pthread_rwlock_wrlock (&unload_prevention_lock)) + abort (); } void unlock_unload (void) { - pthread_rwlock_unlock (&unload_prevention_lock); + if (pthread_rwlock_unlock (&unload_prevention_lock)) + ab...

v1 -> v2: - Fixed everything mentioned in the review. Rich.

...+ 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...

Somewhat invasive but mostly mechanical change to how plugins are called. This patch is in preparation for adding a second backend subtype for filters. Rich.

...n --- src/filters.c | 48 ++++++++++++++++++++++++------------------------ 1 file changed, 24 insertions(+), 24 deletions(-) diff --git a/src/filters.c b/src/filters.c index b68afab..d0692e1 100644 --- a/src/filters.c +++ b/src/filters.c @@ -76,7 +76,7 @@ filter_free (struct backend *b) */ lock_unload (); - debug ("%s: unload", f->filename); + debug ("%s: unload", f->name); if (f->filter.unload) f->filter.unload (); @@ -172,7 +172,7 @@ filter_config (struct backend *b, const char *key, const char *value) struct backend_filter *f = container_of (b,...

...+ /* Acquiring this lock prevents any plugin callbacks from running + * simultaneously. */ - _plugin = plugin_init (); - if (!_plugin) { - fprintf (stderr, "%s: %s: plugin registration function failed\n", - program_name, filename); - exit (EXIT_FAILURE); - } + lock_unload (); - /* Check for incompatible future versions. */ - if (_plugin->_api_version != 1) { - fprintf (stderr, "%s: %s: plugin is incompatible with this version of nbdkit (_api_version = %d)\n", - program_name, filename, _plugin->_api_version); - exit (EXIT_FAILURE...

...Acquiring this lock prevents any plugin callbacks from running + * simultaneously. */ - size = sizeof plugin; /* our struct */ - memset (&plugin, 0, size); - if (size > _plugin->_struct_size) - size = _plugin->_struct_size; - memcpy (&plugin, _plugin, size); + lock_unload (); - /* Check for the minimum fields which must exist in the - * plugin struct. - */ - if (plugin.name == NULL) { - fprintf (stderr, "%s: %s: plugin must have a .name field\n", - program_name, filename); - exit (EXIT_FAILURE); - } - if (plugin.open == NULL) {...

The first three patches are identical to: https://www.redhat.com/archives/libguestfs/2018-January/msg00079.html "[PATCH nbdkit v2 0/3] Refactor plugin_* functions into a backend" The rest of the patches add filters using the new filter API previously described here: https://www.redhat.com/archives/libguestfs/2018-January/msg00073.html This needs a lot more testing -- and tests --

I do have a question about whether patch 2 is right, or whether I've exposed a bigger problem in the truncate (and possibly other) filter, but the rest seem fairly straightforward. Eric Blake (4): server: Check for pthread lock failures truncate: Factor out reading real_size under mutex plugins: Check for mutex failures filters: Check for mutex failures filters/cache/cache.c

...extern void lock_request (struct connection *conn); -extern void unlock_request (struct connection *conn); +extern void lock_request (struct connection *conn) + __attribute__((__nonnull__ (1))); +extern void unlock_request (struct connection *conn) + __attribute__((__nonnull__ (1))); extern void lock_unload (void); extern void unlock_unload (void); /* sockets.c */ -extern int *bind_unix_socket (size_t *); -extern int *bind_tcpip_socket (size_t *); -extern void accept_incoming_connections (int *socks, size_t nr_socks); -extern void free_listening_sockets (int *socks, size_t nr_socks); +extern int *...

v1 was here: https://www.redhat.com/archives/libguestfs/2019-January/msg00008.html In v2 I have provided two patches: The first patch extends attribute((nonnull)) to most internal functions, but not to the external API. The second patch uses a macro so that attribute((format)) is only used in the public API on GCC or Clang. At least in theory these headers could be used by a C compiler which

A combination of the work that both Rich and I have been doing lately, where filters use only the new API with flags on every command that the client can send over the wire (we can then add support for more flags in nbdkit without having to add new callbacks, as NBD adds more flags upstream). Eric Blake (4): protocol: Split flags from cmd field in requests backend: Pass flags argument through

...le chain. */ +static void +filter_free (struct backend *b) +{ + struct backend_filter *f = container_of (b, struct backend_filter, backend); + + f->backend.next->free (f->backend.next); + + /* Acquiring this lock prevents any filter callbacks from running + * simultaneously. + */ + lock_unload (); + + debug ("%s: unload", f->filename); + if (f->filter.unload) + f->filter.unload (); + + dlclose (f->dl); + free (f->filename); + + unlock_unload (); + + free (f); +} + +/* These are actually passing through to the final plugin, hence + * the function names. +...

...the on-load callback if it exists. */ - debug ("%s: load", filename); - if (plugin.load) - plugin.load (); -} - -void -plugin_cleanup (void) -{ - if (dl) { + if (p->dl) { /* Acquiring this lock prevents any plugin callbacks from running * simultaneously. */ lock_unload (); - debug ("%s: unload", filename); - if (plugin.unload) - plugin.unload (); + debug ("%s: unload", p->filename); + if (p->plugin.unload) + p->plugin.unload (); - dlclose (dl); - dl = NULL; - free (filename); - filename = NULL; +...

The immediate reason for this patch is to reduce the amount of debugging in virt-v2v with using the virt-v2v -v option (because this implies running nbdkit in verbose mode too). Most of the messages are datapath ones about pread/pwrite requests, and in fact as we've added more filters on top of nbdkit these messages have got more and more verbose. However they are not particularly

It's easy to use the sh script to demonstrate that nbdkit is inefficiently calling into .get_size, .can_fua, and friends more than necessary. We've also commented on the list in the past that it would be nice to ensure that when filters call into next_ops, they are not violating constraints (as we've have to fix several bugs in the past where we did not have such checking to protect