On Tue, Apr 23, 2019 at 02:06:33PM -0500, Eric Blake wrote: This is a simple code motion, looks fine so: ACK Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com virt-builder quickly builds VMs from scratch http://libguestfs.org/virt-builder.1.html

On 4/23/19 2:06 PM, Eric Blake wrote: Of course, we have to re-add the {} if we fix nbdkit_add_extent() to set reasonable errno so that we can '*err = errno' on failure... -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org

Now that cleanup.h is in common code, we can use it in our filters where it makes sense. Many uses of pthread_mutex_unlock() are not function-wide, and over small enough snippets of code as to be easier to read when left as-is; but when the scope is indeed function-wide or across multiple exit paths, it is nicer to use the macro for automatic unlock. Signed-off-by: Eric Blake <eblake(a)redhat.com&gt; --- filters/log/log.c | 10 ++++------ filters/readahead/readahead.c | 15 +++++---------- filters/log/Makefile.am | 5 ++++- filters/readahead/Makefile.am | 5 ++++- 4 files changed, 17 insertions(+), 18 deletions(-) diff --git a/filters/log/log.c b/filters/log/log.c index 02a2522..513d390 100644 --- a/filters/log/log.c +++ b/filters/log/log.c @@ -45,6 +45,8 @@ #include <nbdkit-filter.h> +#include "cleanup.h" + #define THREAD_MODEL NBDKIT_THREAD_MODEL_PARALLEL static uint64_t connections; @@ -114,12 +116,8 @@ struct handle { static uint64_t get_id (struct handle *h) { - uint64_t r; - - pthread_mutex_lock (&lock); - r = ++h->id; - pthread_mutex_unlock (&lock); - return r; + ACQUIRE_LOCK_FOR_CURRENT_SCOPE(&lock); + return ++h->id; } /* Output a timestamp and the log message. */ diff --git a/filters/readahead/readahead.c b/filters/readahead/readahead.c index 5e14347..f46b6b0 100644 --- a/filters/readahead/readahead.c +++ b/filters/readahead/readahead.c @@ -42,6 +42,7 @@ #include <nbdkit-filter.h> +#include "cleanup.h" #include "minmax.h" #define THREAD_MODEL NBDKIT_THREAD_MODEL_PARALLEL @@ -150,7 +151,7 @@ readahead_pread (struct nbdkit_next_ops *next_ops, void *nxdata, void *handle, void *buf, uint32_t count, uint64_t offset, uint32_t flags, int *err) { - pthread_mutex_lock (&lock); + ACQUIRE_LOCK_FOR_CURRENT_SCOPE (&lock); while (count > 0) { if (length == 0) { @@ -159,7 +160,7 @@ readahead_pread (struct nbdkit_next_ops *next_ops, void *nxdata, */ window = READAHEAD_MIN; if (fill_readahead (next_ops, nxdata, count, offset, flags, err) == -1) - goto err; + return -1; } /* Can we satisfy this request partly or entirely from the prefetch @@ -179,7 +180,7 @@ readahead_pread (struct nbdkit_next_ops *next_ops, void *nxdata, else if (offset == position + length) { window = MIN (window * 2, READAHEAD_MAX); if (fill_readahead (next_ops, nxdata, count, offset, flags, err) == -1) - goto err; + return -1; } /* Else it's a “miss”. Reset everything and start again. */ @@ -187,12 +188,7 @@ readahead_pread (struct nbdkit_next_ops *next_ops, void *nxdata, length = 0; } - pthread_mutex_unlock (&lock); return 0; - - err: - pthread_mutex_unlock (&lock); - return -1; } /* Any writes or write-like operations kill the prefetch buffer. @@ -204,10 +200,9 @@ readahead_pread (struct nbdkit_next_ops *next_ops, void *nxdata, static void kill_readahead (void) { - pthread_mutex_lock (&lock); + ACQUIRE_LOCK_FOR_CURRENT_SCOPE (&lock); window = READAHEAD_MIN; length = 0; - pthread_mutex_unlock (&lock); } static int diff --git a/filters/log/Makefile.am b/filters/log/Makefile.am index 271d54e..08cdd00 100644 --- a/filters/log/Makefile.am +++ b/filters/log/Makefile.am @@ -40,12 +40,15 @@ nbdkit_log_filter_la_SOURCES = \ $(top_srcdir)/include/nbdkit-filter.h nbdkit_log_filter_la_CPPFLAGS = \ - -I$(top_srcdir)/include + -I$(top_srcdir)/include \ + -I$(top_srcdir)/common/utils nbdkit_log_filter_la_CFLAGS = \ $(WARNINGS_CFLAGS) nbdkit_log_filter_la_LDFLAGS = \ -module -avoid-version -shared \ -Wl,--version-script=$(top_srcdir)/filters/filters.syms +nbdkit_log_filter_la_LIBADD = \ + $(top_builddir)/common/utils/libutils.la if HAVE_POD diff --git a/filters/readahead/Makefile.am b/filters/readahead/Makefile.am index 0e7a4a8..54adfb8 100644 --- a/filters/readahead/Makefile.am +++ b/filters/readahead/Makefile.am @@ -41,12 +41,15 @@ nbdkit_readahead_filter_la_SOURCES = \ nbdkit_readahead_filter_la_CPPFLAGS = \ -I$(top_srcdir)/include \ - -I$(top_srcdir)/common/include + -I$(top_srcdir)/common/include \ + -I$(top_srcdir)/common/utils nbdkit_readahead_filter_la_CFLAGS = \ $(WARNINGS_CFLAGS) nbdkit_readahead_filter_la_LDFLAGS = \ -module -avoid-version -shared \ -Wl,--version-script=$(top_srcdir)/filters/filters.syms +nbdkit_readahead_filter_la_LIBADD = \ + $(top_builddir)/common/utils/libutils.la if HAVE_POD -- 2.20.1

Now that cleanup.h is in common code, we can use it in our plugins where it makes sense. Many uses of pthread_mutex_unlock() are not function-wide, and over small enough snippets of code as to be easier to read when left as-is; but when the scope is indeed function-wide or across multiple exit paths, it is nicer to use the macro for automatic unlock. Signed-off-by: Eric Blake <eblake(a)redhat.com&gt; --- plugins/data/data.c | 26 ++++++++------------------ plugins/file/file.c | 18 +++++------------- plugins/memory/memory.c | 26 ++++++++------------------ plugins/nbd/nbd.c | 4 ++-- plugins/data/Makefile.am | 4 +++- plugins/file/Makefile.am | 5 ++++- plugins/memory/Makefile.am | 6 ++++-- plugins/nbd/Makefile.am | 4 +++- 8 files changed, 37 insertions(+), 56 deletions(-) diff --git a/plugins/data/data.c b/plugins/data/data.c index 4f872ce..77cae14 100644 --- a/plugins/data/data.c +++ b/plugins/data/data.c @@ -46,6 +46,7 @@ #include <nbdkit-plugin.h> +#include "cleanup.h" #include "sparse.h" /* If raw|base64|data parameter seen. */ @@ -339,9 +340,8 @@ data_can_multi_conn (void *handle) static int data_pread (void *handle, void *buf, uint32_t count, uint64_t offset) { - pthread_mutex_lock (&lock); + ACQUIRE_LOCK_FOR_CURRENT_SCOPE (&lock); sparse_array_read (sa, buf, count, offset); - pthread_mutex_unlock (&lock); return 0; } @@ -349,21 +349,16 @@ data_pread (void *handle, void *buf, uint32_t count, uint64_t offset) static int data_pwrite (void *handle, const void *buf, uint32_t count, uint64_t offset) { - int r; - - pthread_mutex_lock (&lock); - r = sparse_array_write (sa, buf, count, offset); - pthread_mutex_unlock (&lock); - return r; + ACQUIRE_LOCK_FOR_CURRENT_SCOPE (&lock); + return sparse_array_write (sa, buf, count, offset); } /* Zero. */ static int data_zero (void *handle, uint32_t count, uint64_t offset, int may_trim) { - pthread_mutex_lock (&lock); + ACQUIRE_LOCK_FOR_CURRENT_SCOPE (&lock); sparse_array_zero (sa, count, offset); - pthread_mutex_unlock (&lock); return 0; } @@ -371,9 +366,8 @@ data_zero (void *handle, uint32_t count, uint64_t offset, int may_trim) static int data_trim (void *handle, uint32_t count, uint64_t offset) { - pthread_mutex_lock (&lock); + ACQUIRE_LOCK_FOR_CURRENT_SCOPE (&lock); sparse_array_zero (sa, count, offset); - pthread_mutex_unlock (&lock); return 0; } @@ -382,12 +376,8 @@ static int data_extents (void *handle, uint32_t count, uint64_t offset, uint32_t flags, struct nbdkit_extents *extents) { - int r; - - pthread_mutex_lock (&lock); - r = sparse_array_extents (sa, count, offset, extents); - pthread_mutex_unlock (&lock); - return r; + ACQUIRE_LOCK_FOR_CURRENT_SCOPE (&lock); + return sparse_array_extents (sa, count, offset, extents); } static struct nbdkit_plugin plugin = { diff --git a/plugins/file/file.c b/plugins/file/file.c index 2785399..ebfb71d 100644 --- a/plugins/file/file.c +++ b/plugins/file/file.c @@ -58,6 +58,7 @@ #include <nbdkit-plugin.h> +#include "cleanup.h" #include "isaligned.h" #ifndef O_CLOEXEC @@ -250,12 +251,8 @@ file_get_size (void *handle) struct handle *h = handle; if (h->is_block_device) { - int64_t size; - - pthread_mutex_lock (&lseek_lock); - size = block_device_size (h->fd); - pthread_mutex_unlock (&lseek_lock); - return size; + ACQUIRE_LOCK_FOR_CURRENT_SCOPE (&lseek_lock); + return block_device_size (h->fd); } else { /* Regular file. */ struct stat statbuf; @@ -607,13 +604,8 @@ static int file_extents (void *handle, uint32_t count, uint64_t offset, uint32_t flags, struct nbdkit_extents *extents) { - int r; - - pthread_mutex_lock (&lseek_lock); - r = do_extents (handle, count, offset, flags, extents); - pthread_mutex_unlock (&lseek_lock); - - return r; + ACQUIRE_LOCK_FOR_CURRENT_SCOPE (&lseek_lock); + return do_extents (handle, count, offset, flags, extents); } #endif /* SEEK_HOLE */ diff --git a/plugins/memory/memory.c b/plugins/memory/memory.c index 0685b93..90fa99e 100644 --- a/plugins/memory/memory.c +++ b/plugins/memory/memory.c @@ -45,6 +45,7 @@ #include <nbdkit-plugin.h> +#include "cleanup.h" #include "sparse.h" /* The size of disk in bytes (initialized by size=<SIZE> parameter). */ @@ -131,9 +132,8 @@ memory_can_multi_conn (void *handle) static int memory_pread (void *handle, void *buf, uint32_t count, uint64_t offset) { - pthread_mutex_lock (&lock); + ACQUIRE_LOCK_FOR_CURRENT_SCOPE (&lock); sparse_array_read (sa, buf, count, offset); - pthread_mutex_unlock (&lock); return 0; } @@ -141,21 +141,16 @@ memory_pread (void *handle, void *buf, uint32_t count, uint64_t offset) static int memory_pwrite (void *handle, const void *buf, uint32_t count, uint64_t offset) { - int r; - - pthread_mutex_lock (&lock); - r = sparse_array_write (sa, buf, count, offset); - pthread_mutex_unlock (&lock); - return r; + ACQUIRE_LOCK_FOR_CURRENT_SCOPE (&lock); + return sparse_array_write (sa, buf, count, offset); } /* Zero. */ static int memory_zero (void *handle, uint32_t count, uint64_t offset, int may_trim) { - pthread_mutex_lock (&lock); + ACQUIRE_LOCK_FOR_CURRENT_SCOPE (&lock); sparse_array_zero (sa, count, offset); - pthread_mutex_unlock (&lock); return 0; } @@ -163,9 +158,8 @@ memory_zero (void *handle, uint32_t count, uint64_t offset, int may_trim) static int memory_trim (void *handle, uint32_t count, uint64_t offset) { - pthread_mutex_lock (&lock); + ACQUIRE_LOCK_FOR_CURRENT_SCOPE (&lock); sparse_array_zero (sa, count, offset); - pthread_mutex_unlock (&lock); return 0; } @@ -174,12 +168,8 @@ static int memory_extents (void *handle, uint32_t count, uint64_t offset, uint32_t flags, struct nbdkit_extents *extents) { - int r; - - pthread_mutex_lock (&lock); - r = sparse_array_extents (sa, count, offset, extents); - pthread_mutex_unlock (&lock); - return r; + ACQUIRE_LOCK_FOR_CURRENT_SCOPE (&lock); + return sparse_array_extents (sa, count, offset, extents); } static struct nbdkit_plugin plugin = { diff --git a/plugins/nbd/nbd.c b/plugins/nbd/nbd.c index 57ffc2a..af25a67 100644 --- a/plugins/nbd/nbd.c +++ b/plugins/nbd/nbd.c @@ -50,6 +50,7 @@ #include <nbdkit-plugin.h> #include "protocol.h" #include "byte-swapping.h" +#include "cleanup.h" static char *sockname = NULL; static char *export = NULL; @@ -266,14 +267,13 @@ nbd_request_raw (struct handle *h, uint16_t flags, uint16_t type, }; int r; - pthread_mutex_lock (&h->write_lock); + ACQUIRE_LOCK_FOR_CURRENT_SCOPE (&h->write_lock); nbdkit_debug ("sending request type %d (%s), flags %#x, offset %#" PRIx64 ", count %#x, cookie %#" PRIx64, type, name_of_nbd_cmd(type), flags, offset, count, cookie); r = write_full (h->fd, &req, sizeof req); if (buf && !r) r = write_full (h->fd, buf, count); - pthread_mutex_unlock (&h->write_lock); return r; } diff --git a/plugins/data/Makefile.am b/plugins/data/Makefile.am index 022fdbc..9a36185 100644 --- a/plugins/data/Makefile.am +++ b/plugins/data/Makefile.am @@ -44,7 +44,8 @@ nbdkit_data_plugin_la_SOURCES = \ nbdkit_data_plugin_la_CPPFLAGS = \ -I$(top_srcdir)/include \ -I$(top_srcdir)/common/include \ - -I$(top_srcdir)/common/sparse + -I$(top_srcdir)/common/sparse \ + -I$(top_srcdir)/common/utils nbdkit_data_plugin_la_CFLAGS = \ $(WARNINGS_CFLAGS) \ $(GNUTLS_CFLAGS) @@ -53,6 +54,7 @@ nbdkit_data_plugin_la_LDFLAGS = \ -Wl,--version-script=$(top_srcdir)/plugins/plugins.syms nbdkit_data_plugin_la_LIBADD = \ $(top_builddir)/common/sparse/libsparse.la \ + $(top_builddir)/common/utils/libutils.la \ $(GNUTLS_LIBS) if HAVE_POD diff --git a/plugins/file/Makefile.am b/plugins/file/Makefile.am index f3b3f97..1d28d26 100644 --- a/plugins/file/Makefile.am +++ b/plugins/file/Makefile.am @@ -41,12 +41,15 @@ nbdkit_file_plugin_la_SOURCES = \ nbdkit_file_plugin_la_CPPFLAGS = \ -I$(top_srcdir)/include \ - -I$(top_srcdir)/common/include + -I$(top_srcdir)/common/include \ + -I$(top_srcdir)/common/utils nbdkit_file_plugin_la_CFLAGS = \ $(WARNINGS_CFLAGS) nbdkit_file_plugin_la_LDFLAGS = \ -module -avoid-version -shared \ -Wl,--version-script=$(top_srcdir)/plugins/plugins.syms +nbdkit_file_plugin_la_LIBADD = \ + $(top_builddir)/common/utils/libutils.la if HAVE_POD diff --git a/plugins/memory/Makefile.am b/plugins/memory/Makefile.am index 45df69d..337863e 100644 --- a/plugins/memory/Makefile.am +++ b/plugins/memory/Makefile.am @@ -41,14 +41,16 @@ nbdkit_memory_plugin_la_SOURCES = \ nbdkit_memory_plugin_la_CPPFLAGS = \ -I$(top_srcdir)/include \ - -I$(top_srcdir)/common/sparse + -I$(top_srcdir)/common/sparse \ + -I$(top_srcdir)/common/utils nbdkit_memory_plugin_la_CFLAGS = \ $(WARNINGS_CFLAGS) nbdkit_memory_plugin_la_LDFLAGS = \ -module -avoid-version -shared \ -Wl,--version-script=$(top_srcdir)/plugins/plugins.syms nbdkit_memory_plugin_la_LIBADD = \ - $(top_builddir)/common/sparse/libsparse.la + $(top_builddir)/common/sparse/libsparse.la \ + $(top_builddir)/common/utils/libutils.la if HAVE_POD diff --git a/plugins/nbd/Makefile.am b/plugins/nbd/Makefile.am index b9d486f..7368e59 100644 --- a/plugins/nbd/Makefile.am +++ b/plugins/nbd/Makefile.am @@ -43,6 +43,7 @@ nbdkit_nbd_plugin_la_CPPFLAGS = \ -I$(top_srcdir)/include \ -I$(top_srcdir)/common/include \ -I$(top_srcdir)/common/protocol \ + -I$(top_srcdir)/common/utils \ -I$(top_srcdir)/server nbdkit_nbd_plugin_la_CFLAGS = \ $(WARNINGS_CFLAGS) @@ -50,7 +51,8 @@ nbdkit_nbd_plugin_la_LDFLAGS = \ -module -avoid-version -shared \ -Wl,--version-script=$(top_srcdir)/plugins/plugins.syms nbdkit_nbd_plugin_la_LIBADD = \ - $(top_builddir)/common/protocol/libprotocol.la + $(top_builddir)/common/protocol/libprotocol.la \ + $(top_builddir)/common/utils/libutils.la if HAVE_POD -- 2.20.1