Eric Blake
2019-Apr-23 19:06 UTC
[Libguestfs] [nbdkit PATCH 0/4] Start using cleanup macros in filters/plugins
There's more that can be done (in particular, use of CLEANUP_FREE),
but this is enough to at least see if I'm on the right track.
I couldn't figure out an obvious difference between common/include and
common/utils, but it looks like the former is for things that are
inlineable via .h only, while the latter is when you need to link in
a convenience library, so this landed in the latter.
Eric Blake (4):
cleanup: Move cleanup.c to common
filters: Utilize CLEANUP_EXTENTS_FREE
filters: Utilize ACQUIRE_LOCK_FOR_CURRENT_SCOPE
plugins: Utilize ACQUIRE_LOCK_FOR_CURRENT_SCOPE
common/utils/cleanup.h | 48 ++++++++++++++++++++++++++++++
server/internal.h | 12 +-------
{server => common/utils}/cleanup.c | 5 ++--
filters/log/log.c | 10 +++----
filters/offset/offset.c | 13 ++++----
filters/partition/partition.c | 12 +++-----
filters/readahead/readahead.c | 15 ++++------
filters/truncate/truncate.c | 12 +++-----
plugins/data/data.c | 26 +++++-----------
plugins/file/file.c | 18 ++++-------
plugins/memory/memory.c | 26 +++++-----------
plugins/nbd/nbd.c | 4 +--
common/utils/Makefile.am | 4 ++-
filters/log/Makefile.am | 5 +++-
filters/offset/Makefile.am | 5 +++-
filters/partition/Makefile.am | 5 +++-
filters/readahead/Makefile.am | 5 +++-
filters/truncate/Makefile.am | 5 +++-
plugins/data/Makefile.am | 4 ++-
plugins/file/Makefile.am | 5 +++-
plugins/memory/Makefile.am | 6 ++--
plugins/nbd/Makefile.am | 4 ++-
server/Makefile.am | 7 +++--
23 files changed, 137 insertions(+), 119 deletions(-)
create mode 100644 common/utils/cleanup.h
rename {server => common/utils}/cleanup.c (96%)
--
2.20.1
Eric Blake
2019-Apr-23 19:06 UTC
[Libguestfs] [nbdkit PATCH 1/4] cleanup: Move cleanup.c to common
The CLEANUP_FREE macro and friends can be useful to filters and
in-tree plugins; as such, move them to common/ so more than just the
server/ code can take advantage of our compiler magic.
Signed-off-by: Eric Blake <eblake@redhat.com>
---
common/utils/cleanup.h | 48 ++++++++++++++++++++++++++++++
server/internal.h | 12 +-------
{server => common/utils}/cleanup.c | 5 ++--
common/utils/Makefile.am | 4 ++-
server/Makefile.am | 7 +++--
5 files changed, 58 insertions(+), 18 deletions(-)
create mode 100644 common/utils/cleanup.h
rename {server => common/utils}/cleanup.c (96%)
diff --git a/common/utils/cleanup.h b/common/utils/cleanup.h
new file mode 100644
index 0000000..e6e6140
--- /dev/null
+++ b/common/utils/cleanup.h
@@ -0,0 +1,48 @@
+/* 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.
+ */
+
+#ifndef NBDKIT_CLEANUP_H
+#define NBDKIT_CLEANUP_H
+
+#include <pthread.h>
+
+extern void cleanup_free (void *ptr);
+#define CLEANUP_FREE __attribute__((cleanup (cleanup_free)))
+extern void cleanup_extents_free (void *ptr);
+#define CLEANUP_EXTENTS_FREE __attribute__((cleanup (cleanup_extents_free)))
+extern void cleanup_unlock (pthread_mutex_t **ptr);
+#define CLEANUP_UNLOCK __attribute__((cleanup (cleanup_unlock)))
+#define ACQUIRE_LOCK_FOR_CURRENT_SCOPE(mutex) \
+ CLEANUP_UNLOCK pthread_mutex_t *_lock = mutex; \
+ pthread_mutex_lock (_lock)
+
+#endif /* NBDKIT_CLEANUP_H */
diff --git a/server/internal.h b/server/internal.h
index 817f022..67fccfc 100644
--- a/server/internal.h
+++ b/server/internal.h
@@ -42,6 +42,7 @@
#define NBDKIT_API_VERSION 2
#include "nbdkit-plugin.h"
#include "nbdkit-filter.h"
+#include "cleanup.h"
#ifdef __APPLE__
#define UNIX_PATH_MAX 104
@@ -135,17 +136,6 @@ extern unsigned int get_socket_activation (void);
/* usergroup.c */
extern void change_user (void);
-/* cleanup.c */
-extern void cleanup_free (void *ptr);
-#define CLEANUP_FREE __attribute__((cleanup (cleanup_free)))
-extern void cleanup_extents_free (void *ptr);
-#define CLEANUP_EXTENTS_FREE __attribute__((cleanup (cleanup_extents_free)))
-extern void cleanup_unlock (pthread_mutex_t **ptr);
-#define CLEANUP_UNLOCK __attribute__((cleanup (cleanup_unlock)))
-#define ACQUIRE_LOCK_FOR_CURRENT_SCOPE(mutex) \
- CLEANUP_UNLOCK pthread_mutex_t *_lock = mutex; \
- pthread_mutex_lock (_lock)
-
/* connections.c */
struct connection;
diff --git a/server/cleanup.c b/common/utils/cleanup.c
similarity index 96%
rename from server/cleanup.c
rename to common/utils/cleanup.c
index 292f1ce..196d910 100644
--- a/server/cleanup.c
+++ b/common/utils/cleanup.c
@@ -34,10 +34,9 @@
#include <stdio.h>
#include <stdlib.h>
-#include <stdarg.h>
-#include <string.h>
-#include "internal.h"
+#include "cleanup.h"
+#include "nbdkit-filter.h"
void
cleanup_free (void *ptr)
diff --git a/common/utils/Makefile.am b/common/utils/Makefile.am
index 1773ece..d40d6cf 100644
--- a/common/utils/Makefile.am
+++ b/common/utils/Makefile.am
@@ -34,7 +34,9 @@ include $(top_srcdir)/common-rules.mk
noinst_LTLIBRARIES = libutils.la
libutils_la_SOURCES = \
- utils.c \
+ cleanup.c \
+ cleanup.h \
+ utils.c \
utils.h
libutils_la_CPPFLAGS = \
-I$(top_srcdir)/include
diff --git a/server/Makefile.am b/server/Makefile.am
index 9e13c03..8aa8d3a 100644
--- a/server/Makefile.am
+++ b/server/Makefile.am
@@ -38,7 +38,6 @@ sbin_PROGRAMS = nbdkit
nbdkit_SOURCES = \
background.c \
captive.c \
- cleanup.c \
connections.c \
crypto.c \
debug.c \
@@ -124,9 +123,11 @@ check_PROGRAMS = test-utils
test_utils_SOURCES = \
test-utils.c \
utils.c \
- cleanup.c \
extents.c
test_utils_CPPFLAGS = \
-I$(top_srcdir)/include \
- -I$(top_srcdir)/common/include
+ -I$(top_srcdir)/common/include \
+ -I$(top_srcdir)/common/utils
test_utils_CFLAGS = $(WARNINGS_CFLAGS) $(VALGRIND_CFLAGS)
+test_utils_LDADD = \
+ $(top_builddir)/common/utils/libutils.la
--
2.20.1
Eric Blake
2019-Apr-23 19:06 UTC
[Libguestfs] [nbdkit PATCH 2/4] filters: Utilize CLEANUP_EXTENTS_FREE
Now that cleanup.h is in common code, we can use it in our
filters. The first round focuses just on places that called
nbdkit_extents_free(), as all three callers had multiple exit paths
that definitely benefit from the macro.
Signed-off-by: Eric Blake <eblake@redhat.com>
---
filters/offset/offset.c | 13 +++++--------
filters/partition/partition.c | 12 ++++--------
filters/truncate/truncate.c | 12 ++++--------
filters/offset/Makefile.am | 5 ++++-
filters/partition/Makefile.am | 5 ++++-
filters/truncate/Makefile.am | 5 ++++-
6 files changed, 25 insertions(+), 27 deletions(-)
diff --git a/filters/offset/offset.c b/filters/offset/offset.c
index ebd590b..24ccb4c 100644
--- a/filters/offset/offset.c
+++ b/filters/offset/offset.c
@@ -39,6 +39,8 @@
#include <nbdkit-filter.h>
+#include "cleanup.h"
+
#define THREAD_MODEL NBDKIT_THREAD_MODEL_PARALLEL
static int64_t offset = 0, range = -1;
@@ -138,7 +140,7 @@ offset_extents (struct nbdkit_next_ops *next_ops, void
*nxdata,
struct nbdkit_extents *extents, int *err)
{
size_t i;
- struct nbdkit_extents *extents2;
+ CLEANUP_EXTENTS_FREE struct nbdkit_extents *extents2 = NULL;
struct nbdkit_extent e;
int64_t real_size = next_ops->get_size (nxdata);
@@ -149,20 +151,15 @@ offset_extents (struct nbdkit_next_ops *next_ops, void
*nxdata,
}
if (next_ops->extents (nxdata, count, offs + offset,
flags, extents2, err) == -1)
- goto error;
+ return -1;
for (i = 0; i < nbdkit_extents_count (extents2); ++i) {
e = nbdkit_get_extent (extents2, i);
e.offset -= offset;
if (nbdkit_add_extent (extents, e.offset, e.length, e.type) == -1)
- goto error;
+ return -1;
}
- nbdkit_extents_free (extents2);
return 0;
-
- error:
- nbdkit_extents_free (extents2);
- return -1;
}
static struct nbdkit_filter filter = {
diff --git a/filters/partition/partition.c b/filters/partition/partition.c
index ab692ba..a89dbec 100644
--- a/filters/partition/partition.c
+++ b/filters/partition/partition.c
@@ -41,6 +41,7 @@
#include <nbdkit-filter.h>
#include "byte-swapping.h"
+#include "cleanup.h"
#include "partition.h"
@@ -229,7 +230,7 @@ partition_extents (struct nbdkit_next_ops *next_ops, void
*nxdata,
{
struct handle *h = handle;
size_t i;
- struct nbdkit_extents *extents2;
+ CLEANUP_EXTENTS_FREE struct nbdkit_extents *extents2 = NULL;
struct nbdkit_extent e;
int64_t real_size = next_ops->get_size (nxdata);
@@ -240,20 +241,15 @@ partition_extents (struct nbdkit_next_ops *next_ops, void
*nxdata,
}
if (next_ops->extents (nxdata, count, offs + h->offset,
flags, extents2, err) == -1)
- goto error;
+ return -1;
for (i = 0; i < nbdkit_extents_count (extents2); ++i) {
e = nbdkit_get_extent (extents2, i);
e.offset -= h->offset;
if (nbdkit_add_extent (extents, e.offset, e.length, e.type) == -1)
- goto error;
+ return -1;
}
- nbdkit_extents_free (extents2);
return 0;
-
- error:
- nbdkit_extents_free (extents2);
- return -1;
}
static struct nbdkit_filter filter = {
diff --git a/filters/truncate/truncate.c b/filters/truncate/truncate.c
index dfc6873..076ae22 100644
--- a/filters/truncate/truncate.c
+++ b/filters/truncate/truncate.c
@@ -43,6 +43,7 @@
#include <nbdkit-filter.h>
+#include "cleanup.h"
#include "ispowerof2.h"
#include "iszero.h"
#include "rounding.h"
@@ -292,7 +293,7 @@ truncate_extents (struct nbdkit_next_ops *next_ops, void
*nxdata,
{
uint32_t n;
uint64_t real_size_copy;
- struct nbdkit_extents *extents2;
+ CLEANUP_EXTENTS_FREE struct nbdkit_extents *extents2 = NULL;
size_t i;
pthread_mutex_lock (&lock);
@@ -322,20 +323,15 @@ truncate_extents (struct nbdkit_next_ops *next_ops, void
*nxdata,
n = count;
else
n = real_size_copy - offset;
- if (next_ops->extents (nxdata, n, offset, flags, extents2, err) == -1) {
- nbdkit_extents_free (extents2);
+ if (next_ops->extents (nxdata, n, offset, flags, extents2, err) == -1)
return -1;
- }
for (i = 0; i < nbdkit_extents_count (extents2); ++i) {
struct nbdkit_extent e = nbdkit_get_extent (extents2, i);
- if (nbdkit_add_extent (extents, e.offset, e.length, e.type) == -1) {
- nbdkit_extents_free (extents2);
+ if (nbdkit_add_extent (extents, e.offset, e.length, e.type) == -1)
return -1;
- }
}
- nbdkit_extents_free (extents2);
return 0;
}
diff --git a/filters/offset/Makefile.am b/filters/offset/Makefile.am
index 14591bb..525d9b6 100644
--- a/filters/offset/Makefile.am
+++ b/filters/offset/Makefile.am
@@ -40,12 +40,15 @@ nbdkit_offset_filter_la_SOURCES = \
$(top_srcdir)/include/nbdkit-filter.h
nbdkit_offset_filter_la_CPPFLAGS = \
- -I$(top_srcdir)/include
+ -I$(top_srcdir)/include \
+ -I$(top_srcdir)/common/utils
nbdkit_offset_filter_la_CFLAGS = \
$(WARNINGS_CFLAGS)
nbdkit_offset_filter_la_LDFLAGS = \
-module -avoid-version -shared \
-Wl,--version-script=$(top_srcdir)/filters/filters.syms
+nbdkit_offset_filter_la_LIBADD = \
+ $(top_builddir)/common/utils/libutils.la
if HAVE_POD
diff --git a/filters/partition/Makefile.am b/filters/partition/Makefile.am
index 6fbbe17..f335bdc 100644
--- a/filters/partition/Makefile.am
+++ b/filters/partition/Makefile.am
@@ -45,12 +45,15 @@ nbdkit_partition_filter_la_SOURCES = \
nbdkit_partition_filter_la_CPPFLAGS = \
-I$(top_srcdir)/include \
-I$(top_srcdir)/common/gpt \
- -I$(top_srcdir)/common/include
+ -I$(top_srcdir)/common/include \
+ -I$(top_srcdir)/common/utils
nbdkit_partition_filter_la_CFLAGS = \
$(WARNINGS_CFLAGS)
nbdkit_partition_filter_la_LDFLAGS = \
-module -avoid-version -shared \
-Wl,--version-script=$(top_srcdir)/filters/filters.syms
+nbdkit_partition_filter_la_LIBADD = \
+ $(top_builddir)/common/utils/libutils.la
if HAVE_POD
diff --git a/filters/truncate/Makefile.am b/filters/truncate/Makefile.am
index 86709d4..c591703 100644
--- a/filters/truncate/Makefile.am
+++ b/filters/truncate/Makefile.am
@@ -41,12 +41,15 @@ nbdkit_truncate_filter_la_SOURCES = \
nbdkit_truncate_filter_la_CPPFLAGS = \
-I$(top_srcdir)/include \
- -I$(top_srcdir)/common/include
+ -I$(top_srcdir)/common/include \
+ -I$(top_srcdir)/common/utils
nbdkit_truncate_filter_la_CFLAGS = \
$(WARNINGS_CFLAGS)
nbdkit_truncate_filter_la_LDFLAGS = \
-module -avoid-version -shared \
-Wl,--version-script=$(top_srcdir)/filters/filters.syms
+nbdkit_truncate_filter_la_LIBADD = \
+ $(top_builddir)/common/utils/libutils.la
if HAVE_POD
--
2.20.1
Eric Blake
2019-Apr-23 19:06 UTC
[Libguestfs] [nbdkit PATCH 3/4] filters: Utilize ACQUIRE_LOCK_FOR_CURRENT_SCOPE
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@redhat.com>
---
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
Eric Blake
2019-Apr-23 19:06 UTC
[Libguestfs] [nbdkit PATCH 4/4] plugins: Utilize ACQUIRE_LOCK_FOR_CURRENT_SCOPE
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@redhat.com>
---
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
Eric Blake
2019-Apr-23 19:11 UTC
Re: [Libguestfs] [nbdkit PATCH 2/4] filters: Utilize CLEANUP_EXTENTS_FREE
On 4/23/19 2:06 PM, Eric Blake wrote:> Now that cleanup.h is in common code, we can use it in our > filters. The first round focuses just on places that called > nbdkit_extents_free(), as all three callers had multiple exit paths > that definitely benefit from the macro. > > Signed-off-by: Eric Blake <eblake@redhat.com> > --- > filters/offset/offset.c | 13 +++++-------- > filters/partition/partition.c | 12 ++++-------- > filters/truncate/truncate.c | 12 ++++-------- > filters/offset/Makefile.am | 5 ++++- > filters/partition/Makefile.am | 5 ++++- > filters/truncate/Makefile.am | 5 ++++- > 6 files changed, 25 insertions(+), 27 deletions(-) >> @@ -322,20 +323,15 @@ truncate_extents (struct nbdkit_next_ops *next_ops, void *nxdata, > n = count; > else > n = real_size_copy - offset; > - if (next_ops->extents (nxdata, n, offset, flags, extents2, err) == -1) { > - nbdkit_extents_free (extents2); > + if (next_ops->extents (nxdata, n, offset, flags, extents2, err) == -1) > return -1; > - } > > for (i = 0; i < nbdkit_extents_count (extents2); ++i) { > struct nbdkit_extent e = nbdkit_get_extent (extents2, i); > > - if (nbdkit_add_extent (extents, e.offset, e.length, e.type) == -1) { > - nbdkit_extents_free (extents2); > + if (nbdkit_add_extent (extents, e.offset, e.length, e.type) == -1) > return -1; > - }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
Richard W.M. Jones
2019-Apr-24 07:42 UTC
Re: [Libguestfs] [nbdkit PATCH 1/4] cleanup: Move cleanup.c to common
On Tue, Apr 23, 2019 at 02:06:33PM -0500, Eric Blake wrote:> The CLEANUP_FREE macro and friends can be useful to filters and > in-tree plugins; as such, move them to common/ so more than just the > server/ code can take advantage of our compiler magic. > > Signed-off-by: Eric Blake <eblake@redhat.com> > --- > common/utils/cleanup.h | 48 ++++++++++++++++++++++++++++++ > server/internal.h | 12 +------- > {server => common/utils}/cleanup.c | 5 ++-- > common/utils/Makefile.am | 4 ++- > server/Makefile.am | 7 +++-- > 5 files changed, 58 insertions(+), 18 deletions(-) > create mode 100644 common/utils/cleanup.h > rename {server => common/utils}/cleanup.c (96%) > > diff --git a/common/utils/cleanup.h b/common/utils/cleanup.h > new file mode 100644 > index 0000000..e6e6140 > --- /dev/null > +++ b/common/utils/cleanup.h > @@ -0,0 +1,48 @@ > +/* 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. > + */ > + > +#ifndef NBDKIT_CLEANUP_H > +#define NBDKIT_CLEANUP_H > + > +#include <pthread.h> > + > +extern void cleanup_free (void *ptr); > +#define CLEANUP_FREE __attribute__((cleanup (cleanup_free))) > +extern void cleanup_extents_free (void *ptr); > +#define CLEANUP_EXTENTS_FREE __attribute__((cleanup (cleanup_extents_free))) > +extern void cleanup_unlock (pthread_mutex_t **ptr); > +#define CLEANUP_UNLOCK __attribute__((cleanup (cleanup_unlock))) > +#define ACQUIRE_LOCK_FOR_CURRENT_SCOPE(mutex) \ > + CLEANUP_UNLOCK pthread_mutex_t *_lock = mutex; \ > + pthread_mutex_lock (_lock) > + > +#endif /* NBDKIT_CLEANUP_H */ > diff --git a/server/internal.h b/server/internal.h > index 817f022..67fccfc 100644 > --- a/server/internal.h > +++ b/server/internal.h > @@ -42,6 +42,7 @@ > #define NBDKIT_API_VERSION 2 > #include "nbdkit-plugin.h" > #include "nbdkit-filter.h" > +#include "cleanup.h" > > #ifdef __APPLE__ > #define UNIX_PATH_MAX 104 > @@ -135,17 +136,6 @@ extern unsigned int get_socket_activation (void); > /* usergroup.c */ > extern void change_user (void); > > -/* cleanup.c */ > -extern void cleanup_free (void *ptr); > -#define CLEANUP_FREE __attribute__((cleanup (cleanup_free))) > -extern void cleanup_extents_free (void *ptr); > -#define CLEANUP_EXTENTS_FREE __attribute__((cleanup (cleanup_extents_free))) > -extern void cleanup_unlock (pthread_mutex_t **ptr); > -#define CLEANUP_UNLOCK __attribute__((cleanup (cleanup_unlock))) > -#define ACQUIRE_LOCK_FOR_CURRENT_SCOPE(mutex) \ > - CLEANUP_UNLOCK pthread_mutex_t *_lock = mutex; \ > - pthread_mutex_lock (_lock) > - > /* connections.c */ > struct connection; > > diff --git a/server/cleanup.c b/common/utils/cleanup.c > similarity index 96% > rename from server/cleanup.c > rename to common/utils/cleanup.c > index 292f1ce..196d910 100644 > --- a/server/cleanup.c > +++ b/common/utils/cleanup.c > @@ -34,10 +34,9 @@ > > #include <stdio.h> > #include <stdlib.h> > -#include <stdarg.h> > -#include <string.h> > > -#include "internal.h" > +#include "cleanup.h" > +#include "nbdkit-filter.h" > > void > cleanup_free (void *ptr) > diff --git a/common/utils/Makefile.am b/common/utils/Makefile.am > index 1773ece..d40d6cf 100644 > --- a/common/utils/Makefile.am > +++ b/common/utils/Makefile.am > @@ -34,7 +34,9 @@ include $(top_srcdir)/common-rules.mk > noinst_LTLIBRARIES = libutils.la > > libutils_la_SOURCES = \ > - utils.c \ > + cleanup.c \ > + cleanup.h \ > + utils.c \ > utils.h > libutils_la_CPPFLAGS = \ > -I$(top_srcdir)/include > diff --git a/server/Makefile.am b/server/Makefile.am > index 9e13c03..8aa8d3a 100644 > --- a/server/Makefile.am > +++ b/server/Makefile.am > @@ -38,7 +38,6 @@ sbin_PROGRAMS = nbdkit > nbdkit_SOURCES = \ > background.c \ > captive.c \ > - cleanup.c \ > connections.c \ > crypto.c \ > debug.c \ > @@ -124,9 +123,11 @@ check_PROGRAMS = test-utils > test_utils_SOURCES = \ > test-utils.c \ > utils.c \ > - cleanup.c \ > extents.c > test_utils_CPPFLAGS = \ > -I$(top_srcdir)/include \ > - -I$(top_srcdir)/common/include > + -I$(top_srcdir)/common/include \ > + -I$(top_srcdir)/common/utils > test_utils_CFLAGS = $(WARNINGS_CFLAGS) $(VALGRIND_CFLAGS) > +test_utils_LDADD = \ > + $(top_builddir)/common/utils/libutils.laThis 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
Richard W.M. Jones
2019-Apr-24 08:00 UTC
Re: [Libguestfs] [nbdkit PATCH 3/4] filters: Utilize ACQUIRE_LOCK_FOR_CURRENT_SCOPE
Yes this makes the code a lot neater, 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-df lists disk usage of guests without needing to install any software inside the virtual machine. Supports Linux and Windows. http://people.redhat.com/~rjones/virt-df/
Richard W.M. Jones
2019-Apr-24 08:01 UTC
Re: [Libguestfs] [nbdkit PATCH 4/4] plugins: Utilize ACQUIRE_LOCK_FOR_CURRENT_SCOPE
Again much nicer: ACK series I will push this in a moment. Thanks, 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
Possibly Parallel Threads
- Re: [nbdkit PATCH 2/4] filters: Utilize CLEANUP_EXTENTS_FREE
- [PATCH nbdkit v5 FINAL 12/19] truncate: Implement extents for beyond end of truncated region.
- [PATCH nbdkit 2/2] filters: Be careful to set *err if nbdkit_add_extent or nbdkit_extents_new fail.
- [nbdkit PATCH 2/3] extents: Add nbdkit_extents_aligned()
- Re: [PATCH nbdkit v5 FINAL 12/19] truncate: Implement extents for beyond end of truncated region.