[PATCH common] mlutils: Simple wrapper around sysconf (_SC_NPROCESSORS_ONLN).
by Richard W.M. Jones
---
mlutils/unix_utils-c.c | 15 +++++++++++++++
mlutils/unix_utils.ml | 5 +++++
mlutils/unix_utils.mli | 9 +++++++++
3 files changed, 29 insertions(+)
diff --git a/mlutils/unix_utils-c.c b/mlutils/unix_utils-c.c
index 3309961..8acf039 100644
--- a/mlutils/unix_utils-c.c
+++ b/mlutils/unix_utils-c.c
@@ -77,6 +77,7 @@ extern value guestfs_int_mllib_mkdtemp (value val_pattern);
extern value guestfs_int_mllib_realpath (value pathv);
extern value guestfs_int_mllib_statvfs_statvfs (value pathv);
extern value guestfs_int_mllib_statvfs_is_network_filesystem (value pathv);
+extern value guestfs_int_mllib_sysconf_nr_processors_online (value unitv);
/* NB: This is a "noalloc" call. */
value
@@ -368,3 +369,17 @@ guestfs_int_mllib_statvfs_is_network_filesystem (value pathv)
return Val_bool (0);
#endif
}
+
+/* NB: This is a "noalloc" call. */
+value
+guestfs_int_mllib_sysconf_nr_processors_online (value unitv)
+{
+#ifdef _SC_NPROCESSORS_ONLN
+ long n;
+
+ n = sysconf (_SC_NPROCESSORS_ONLN);
+ if (n > 0) return Val_int (n);
+#endif
+ /* Return a safe value so that callers don't need to deal with errors. */
+ return Val_int (1);
+}
diff --git a/mlutils/unix_utils.ml b/mlutils/unix_utils.ml
index 52eb824..2bdda12 100644
--- a/mlutils/unix_utils.ml
+++ b/mlutils/unix_utils.ml
@@ -84,3 +84,8 @@ module StatVFS = struct
external is_network_filesystem : string -> bool =
"guestfs_int_mllib_statvfs_is_network_filesystem" "noalloc"
end
+
+module Sysconf = struct
+ external nr_processors_online : unit -> int =
+ "guestfs_int_mllib_sysconf_nr_processors_online" "noalloc"
+end
diff --git a/mlutils/unix_utils.mli b/mlutils/unix_utils.mli
index 4fcea4a..aead4df 100644
--- a/mlutils/unix_utils.mli
+++ b/mlutils/unix_utils.mli
@@ -121,3 +121,12 @@ module StatVFS : sig
(** [is_network_filesystem path] returns true if [path] is located on
a network filesystem such as NFS or CIFS. *)
end
+
+module Sysconf : sig
+ val nr_processors_online : unit -> int
+ (** [nr_processors_online ()] returns the number of processors
+ currently online, from [sysconf (_SC_NPROCESSORS_ONLN)].
+
+ Note this never fails. In case we cannot get the number of
+ cores it returns 1. *)
+end
--
2.27.0
3 years, 8 months
[libnbd PATCH] api: Add LIBNBD_SHUTDOWN_IMMEDIATE flag
by Eric Blake
As mentioned in commits 176fc4ea and 609c25f0, our original plan in
adding a flags argument to nbd_shutdown was to let us specify
different behaviors at the libnbd level, rather than NBD protocol
flags (for that, the user has nbd_aio_disconnect). But when we later
parameterized OFlags to accept various bitmasks (commit f891340b), we
failed to mark nbd_shutdown as using a different bitmask than
NBD_CMD_FLAG.
I've finally found a use for such a flag. By itself,
nbd_aio_disconnect merely queues itself at the back of all pending
commands. But if the server is processing responses slowly, it can be
desirable to elevate a disconnect request to the front of the queue,
intentionally abandoning all subsequent commands that have not already
started on their way to the server.
Fortunately, we have been rejecting all flag values, so changing the
type of the OFlags argument now has no impact to either the C API or
the ABI. Other language bindings that are more strongly-typed will
see a different enum, but we don't promise compatibility there, and
even then, languages that permit omitting the flags argument in favor
of a default don't see any difference for client that were omitting
the argument in favor of the default.
Note that the new LIBNBD_SHUTDOWN_IMMEDIATE flag is not necessarily
instant: if the server is still receiving the previous command, we
have to wait for that before the server can learn of our intent, and
the command itself still blocks until we enter closed or dead states.
But it can certainly reduce the time spent in nbd_shutdown by not
having to wait for all unsent commands in the queue to also be
processed by the server.
---
lib/internal.h | 2 +
generator/API.ml | 28 ++++++-
generator/states.c | 12 +--
lib/disconnect.c | 16 +++-
tests/Makefile.am | 7 ++
tests/shutdown-flags.c | 166 +++++++++++++++++++++++++++++++++++++++++
.gitignore | 1 +
7 files changed, 221 insertions(+), 11 deletions(-)
create mode 100644 tests/shutdown-flags.c
diff --git a/lib/internal.h b/lib/internal.h
index b2637bd..96699b5 100644
--- a/lib/internal.h
+++ b/lib/internal.h
@@ -435,6 +435,8 @@ extern int64_t nbd_internal_command_common (struct nbd_handle *h,
struct socket *nbd_internal_socket_create (int fd);
/* states.c */
+extern void nbd_internal_abort_commands (struct nbd_handle *h,
+ struct command **list);
extern int nbd_internal_run (struct nbd_handle *h, enum external_event ev);
extern const char *nbd_internal_state_short_string (enum state state);
extern enum state_group nbd_internal_state_group (enum state state);
diff --git a/generator/API.ml b/generator/API.ml
index 1d920cf..6cdab34 100644
--- a/generator/API.ml
+++ b/generator/API.ml
@@ -181,7 +181,14 @@ let allow_transport_flags = {
"VSOCK", 1 lsl 2;
]
}
-let all_flags = [ cmd_flags; handshake_flags; allow_transport_flags ]
+let shutdown_flags = {
+ flag_prefix = "SHUTDOWN";
+ flags = [
+ "IMMEDIATE", 1 lsl 1;
+ ]
+}
+let all_flags = [ cmd_flags; handshake_flags; allow_transport_flags;
+ shutdown_flags]
let default_call = { args = []; optargs = []; ret = RErr;
shortdesc = ""; longdesc = ""; example = None;
@@ -1595,7 +1602,7 @@ L<nbd_can_fua(3)>).";
"shutdown", {
default_call with
- args = []; optargs = [ OFlags ("flags", cmd_flags) ]; ret = RErr;
+ args = []; optargs = [ OFlags ("flags", shutdown_flags) ]; ret = RErr;
permitted_states = [ Connected ];
shortdesc = "disconnect from the NBD server";
longdesc = "\
@@ -1608,8 +1615,21 @@ This function works whether or not the handle is ready for
transmission of commands. If more fine-grained control is
needed, see L<nbd_aio_disconnect(3)>.
-The C<flags> parameter must be C<0> for now (it exists for future NBD
-protocol extensions).";
+The C<flags> argument is a bitmask, including zero or more of the
+following shutdown flags:
+
+=over 4
+
+=item C<LIBNBD_SHUTDOWN_IMMEDIATE> = 1
+
+If there are any pending requests which have not yet been sent to
+the server (see L<nbd_aio_in_flight(3)>), abandon them without
+sending them to the server, rather than the usual practice of
+issuing those commands before informing the server of the intent
+to disconnect.
+
+=back
+";
see_also = [Link "close"; Link "aio_disconnect"];
example = Some "examples/reads-and-writes.c";
};
diff --git a/generator/states.c b/generator/states.c
index 9a12e82..0ecba7e 100644
--- a/generator/states.c
+++ b/generator/states.c
@@ -122,8 +122,8 @@ abort_option (struct nbd_handle *h)
}
/* Forcefully fail any remaining in-flight commands in list */
-static void
-abort_commands (struct nbd_handle *h, struct command **list)
+void
+nbd_internal_abort_commands (struct nbd_handle *h, struct command **list)
{
struct command *next, *cmd;
@@ -179,8 +179,8 @@ STATE_MACHINE {
/* The caller should have used set_error() before reaching here */
assert (nbd_get_error ());
abort_option (h);
- abort_commands (h, &h->cmds_to_issue);
- abort_commands (h, &h->cmds_in_flight);
+ nbd_internal_abort_commands (h, &h->cmds_to_issue);
+ nbd_internal_abort_commands (h, &h->cmds_in_flight);
h->in_flight = 0;
if (h->sock) {
h->sock->ops->close (h->sock);
@@ -190,8 +190,8 @@ STATE_MACHINE {
CLOSED:
abort_option (h);
- abort_commands (h, &h->cmds_to_issue);
- abort_commands (h, &h->cmds_in_flight);
+ nbd_internal_abort_commands (h, &h->cmds_to_issue);
+ nbd_internal_abort_commands (h, &h->cmds_in_flight);
h->in_flight = 0;
if (h->sock) {
h->sock->ops->close (h->sock);
diff --git a/lib/disconnect.c b/lib/disconnect.c
index dcb95d8..b8356b7 100644
--- a/lib/disconnect.c
+++ b/lib/disconnect.c
@@ -23,17 +23,31 @@
#include <stdbool.h>
#include <errno.h>
#include <inttypes.h>
+#include <assert.h>
#include "internal.h"
int
nbd_unlocked_shutdown (struct nbd_handle *h, uint32_t flags)
{
- if (flags != 0) {
+ if ((flags & ~LIBNBD_SHUTDOWN_IMMEDIATE) != 0) {
set_error (EINVAL, "invalid flag: %" PRIu32, flags);
return -1;
}
+ /* If IMMEDIATE, abort any commands that have not yet had any bytes
+ * sent to the server, so that NBD_CMD_DISC will be first in line.
+ */
+ if (flags & LIBNBD_SHUTDOWN_IMMEDIATE) {
+ struct command **cmd = &h->cmds_to_issue;
+ if (!nbd_internal_is_state_ready (get_next_state (h))) {
+ assert (*cmd);
+ h->cmds_to_issue_tail = *cmd;
+ cmd = &((*cmd)->next);
+ }
+ nbd_internal_abort_commands (h, cmd);
+ }
+
if (!h->disconnect_request &&
(nbd_internal_is_state_ready (get_next_state (h)) ||
nbd_internal_is_state_processing (get_next_state (h)))) {
diff --git a/tests/Makefile.am b/tests/Makefile.am
index be7c83c..007c69e 100644
--- a/tests/Makefile.am
+++ b/tests/Makefile.am
@@ -142,6 +142,7 @@ if HAVE_NBDKIT
check_PROGRAMS += \
errors \
server-death \
+ shutdown-flags \
get-size \
read-only-flag \
read-write-flag \
@@ -180,6 +181,7 @@ check_PROGRAMS += \
TESTS += \
errors \
server-death \
+ shutdown-flags \
get-size \
read-only-flag \
read-write-flag \
@@ -225,6 +227,11 @@ server_death_CPPFLAGS = -I$(top_srcdir)/include
server_death_CFLAGS = $(WARNINGS_CFLAGS)
server_death_LDADD = $(top_builddir)/lib/libnbd.la
+shutdown_flags_SOURCES = shutdown-flags.c
+shutdown_flags_CPPFLAGS = -I$(top_srcdir)/include
+shutdown_flags_CFLAGS = $(WARNINGS_CFLAGS)
+shutdown_flags_LDADD = $(top_builddir)/lib/libnbd.la
+
get_size_SOURCES = get-size.c
get_size_CPPFLAGS = -I$(top_srcdir)/include
get_size_CFLAGS = $(WARNINGS_CFLAGS)
diff --git a/tests/shutdown-flags.c b/tests/shutdown-flags.c
new file mode 100644
index 0000000..6043ee2
--- /dev/null
+++ b/tests/shutdown-flags.c
@@ -0,0 +1,166 @@
+/* NBD client library in userspace
+ * Copyright (C) 2013-2020 Red Hat Inc.
+ *
+ * This library is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public
+ * License as published by the Free Software Foundation; either
+ * version 2 of the License, or (at your option) any later version.
+ *
+ * This library is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public
+ * License along with this library; if not, write to the Free Software
+ * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA
+ */
+
+/* Deliberately shutdown while stranding commands, to check their status.
+ */
+
+#include <config.h>
+
+#include <stdio.h>
+#include <stdlib.h>
+#include <string.h>
+#include <errno.h>
+#include <signal.h>
+
+#include <libnbd.h>
+
+static bool write_retired;
+static const char *progname;
+
+static int
+callback (void *ignored, int *error)
+{
+ if (*error != ENOTCONN) {
+ fprintf (stderr, "%s: unexpected error in pwrite callback: %s\n",
+ progname, strerror (*error));
+ return 0;
+ }
+ write_retired = 1;
+ return 1;
+}
+
+static char buf[2 * 1024 * 1024];
+
+int
+main (int argc, char *argv[])
+{
+ struct nbd_handle *nbd;
+ int err;
+ const char *msg;
+ int64_t cookie;
+ const char *cmd[] = { "nbdkit", "-s", "--exit-with-parent",
+ "memory", "size=2m", NULL };
+
+ progname = argv[0];
+
+ nbd = nbd_create ();
+ if (nbd == NULL) {
+ fprintf (stderr, "%s: %s\n", argv[0], nbd_get_error ());
+ exit (EXIT_FAILURE);
+ }
+
+ /* Connect to a server. */
+ if (nbd_connect_command (nbd, (char **) cmd) == -1) {
+ fprintf (stderr, "%s: %s\n", argv[0], nbd_get_error ());
+ exit (EXIT_FAILURE);
+ }
+
+ /* Pause the server from reading, so that our first request will
+ * exceed the buffer and force the second request to be stuck client
+ * side (without stopping the server, we would be racing on whether
+ * we hit a block on writes based on whether the server can read
+ * faster than we can fill the pipe).
+ */
+ if (nbd_kill_subprocess (nbd, SIGSTOP) == -1) {
+ fprintf (stderr, "%s: test failed: nbd_kill_subprocess: %s\n", argv[0],
+ nbd_get_error ());
+ exit (EXIT_FAILURE);
+ }
+
+ /* Issue back-to-back write requests, both large enough to block. Set up
+ * the second to auto-retire via callback.
+ */
+ if ((cookie = nbd_aio_pwrite (nbd, buf, sizeof buf, 0,
+ NBD_NULL_COMPLETION, 0)) == -1) {
+ fprintf (stderr, "%s: test failed: first nbd_aio_pwrite: %s\n", argv[0],
+ nbd_get_error ());
+ exit (EXIT_FAILURE);
+ }
+ if (nbd_aio_pwrite (nbd, buf, sizeof buf, 0,
+ (nbd_completion_callback) { .callback = callback },
+ 0) == -1) {
+ fprintf (stderr, "%s: test failed: second nbd_aio_pwrite: %s\n", argv[0],
+ nbd_get_error ());
+ exit (EXIT_FAILURE);
+ }
+
+ /* Resume the server; but now our state machine remains blocked
+ * until we notify or otherwise poll it.
+ */
+ if (nbd_kill_subprocess (nbd, SIGCONT) == -1) {
+ fprintf (stderr, "%s: test failed: nbd_kill_subprocess: %s\n", argv[0],
+ nbd_get_error ());
+ exit (EXIT_FAILURE);
+ }
+ if (nbd_aio_peek_command_completed (nbd) != 0) {
+ fprintf (stderr, "%s: test failed: nbd_aio_peek_command_completed\n",
+ argv[0]);
+ exit (EXIT_FAILURE);
+ }
+ if (nbd_aio_command_completed (nbd, cookie) != 0) {
+ fprintf (stderr, "%s: test failed: nbd_aio_command_completed\n", argv[0]);
+ exit (EXIT_FAILURE);
+ }
+
+ /* Send an immediate shutdown. This will abort the second write, as
+ * well as kick the state machine to finish the first.
+ */
+ if (nbd_shutdown (nbd, LIBNBD_SHUTDOWN_IMMEDIATE) == -1) {
+ fprintf (stderr, "%s: test failed: nbd_shutdown\n", argv[0]);
+ exit (EXIT_FAILURE);
+ }
+
+ /* All in-flight commands should now be completed. But whether the
+ * first write succeeded or failed depends on the server, so we
+ * merely retire it without checking status.
+ */
+ if (nbd_aio_in_flight (nbd) != 0) {
+ fprintf (stderr, "%s: test failed: nbd_aio_in_flight\n", argv[0]);
+ exit (EXIT_FAILURE);
+ }
+ if (nbd_aio_peek_command_completed (nbd) != cookie) {
+ fprintf (stderr, "%s: test failed: nbd_aio_peek_command_completed\n",
+ argv[0]);
+ exit (EXIT_FAILURE);
+ }
+ nbd_aio_command_completed (nbd, cookie);
+
+ /* With all commands retired, no further command should be pending */
+ if (!write_retired) {
+ fprintf (stderr, "%s: test failed: second nbd_aio_pwrite not retired\n",
+ argv[0]);
+ exit (EXIT_FAILURE);
+ }
+ if (nbd_aio_peek_command_completed (nbd) != -1) {
+ fprintf (stderr, "%s: test failed: nbd_aio_peek_command_completed\n",
+ argv[0]);
+ exit (EXIT_FAILURE);
+ }
+ msg = nbd_get_error ();
+ err = nbd_get_errno ();
+ printf ("error: \"%s\"\n", msg);
+ printf ("errno: %d (%s)\n", err, strerror (err));
+ if (err != EINVAL) {
+ fprintf (stderr, "%s: test failed: unexpected errno %d (%s)\n", argv[0],
+ err, strerror (err));
+ exit (EXIT_FAILURE);
+ }
+
+ nbd_close (nbd);
+ exit (EXIT_SUCCESS);
+}
diff --git a/.gitignore b/.gitignore
index 6812fe8..aa9e0bb 100644
--- a/.gitignore
+++ b/.gitignore
@@ -179,6 +179,7 @@ Makefile.in
/tests/read-only-flag
/tests/read-write-flag
/tests/server-death
+/tests/shutdown-flags
/tests/synch-parallel
/tests/synch-parallel-tls
/tests/version
--
2.28.0
3 years, 8 months
[libnbd PATCH v2 0/5] Add knobs for client- vs. server-side validation
by Eric Blake
In v2:
- now based on my proposal to add LIBNBD_SHUTDOWN_IMMEDIATE
- four flags instead of two: STRICT_FLAGS is new (patch 4),
and STRICT_BOUNDS is separate from STRICT_ZERO_SIZE (patch 5)
- various refactorings for more shared code and less duplication
Eric Blake (5):
api: Add xxx_MASK constant for each Flags type
generator: Refactor filtering of accepted OFlags
api: Add nbd_set_strict_mode
api: Add STRICT_FLAGS to set_strict_mode
api: Add STRICT_BOUNDS/ZERO_SIZE to nbd_set_strict_mode
docs/libnbd.pod | 7 +
lib/internal.h | 6 +-
generator/API.ml | 291 ++++++++++++++----
generator/API.mli | 4 +-
generator/C.ml | 38 ++-
generator/GoLang.ml | 18 +-
generator/OCaml.ml | 15 +-
generator/Python.ml | 15 +-
lib/disconnect.c | 13 +-
lib/handle.c | 21 +-
lib/rw.c | 232 ++++++--------
python/t/110-defaults.py | 3 +-
python/t/120-set-non-defaults.py | 5 +-
ocaml/tests/test_110_defaults.ml | 3 +-
ocaml/tests/test_120_set_non_defaults.ml | 3 +-
tests/errors.c | 72 ++++-
.../libnbd/libnbd_110_defaults_test.go | 2 +-
.../libnbd_120_set_non_defaults_test.go | 4 +-
18 files changed, 488 insertions(+), 264 deletions(-)
--
2.28.0
3 years, 8 months
[PATCH v2 0/7] Windows BitLocker support.
by Richard W.M. Jones
Original version linked from here:
https://bugzilla.redhat.com/show_bug.cgi?id=1808977#c8
There is no change in the code in this series, but feedback from the
original series was we shouldn't lose the error message in patch 7.
When I tested this just now in fact we don't lose the error if
debugging is enabled, but I have updated the commit message to note
what the error message is in the BitLocker case.
Rich.
3 years, 8 months
[PATCH v2v] Add ALT support
by Richard W.M. Jones
Patch supplied by Mikhail Gordeev, posting for review.
I have compile tested it and checked the code and it looks all
fine to me, so ACK from my point of view. I did not actually
run it because I don't have an ALT Linux install, but it
doesn't seem as if it would affect any other distro.
Rich.
3 years, 8 months
[libnbd PATCH] python: Fix more memory leaks
by Eric Blake
h.nbd_connect_command was leaking a python object per parameter, and
also leaks memory on failure. In fact, it was possible to segfault on
something as trivial as:
nbdsh -c 'h.connect_command(["true",1])'
h.nbd_pread was leaking a read buffer on every single synchronous read.
My previous patch to address closure callbacks had a bug in
h.nbd_pread_structured and similar: if the allocation for Closure
fails, we hit 'goto err' prior to initializing the OClosure pointers,
which could lead to freeing garbage. Similarly, when messing with
non-callable for either the Closure or the OClosure, there was enough
inaccuracy in python reference counting to cause a leak or
double-free.
We were not consistently checking for python function failures (such
as when a wrong type is passed in).
While touching this, drop a pointless cast to char* when calling
PyArg_ParseTuple, and rearrange some of the code for fewer loops over
args/optargs in the generator.
Fixes: 4fab2104ab
Fixes: bf0eee111f
Fixes: 82dac49af0
---
I'm pushing this as followup to my previous patch.
I'm pretty embarrassed that we've let nbdsh leak as many bytes as were
read via h.pread(), all the way since v0.1; that sort of leakage ought
to be easier to detect. But using valgrind on python is a bit
difficult, to ferret out the real leaks vs. the python garbage
collection reference chain.
generator/Python.ml | 103 +++++++++++++++++++++-----------------------
python/handle.c | 5 ++-
python/utils.c | 11 ++++-
3 files changed, 63 insertions(+), 56 deletions(-)
diff --git a/generator/Python.ml b/generator/Python.ml
index 9a22f9e..3b86dc0 100644
--- a/generator/Python.ml
+++ b/generator/Python.ml
@@ -271,7 +271,7 @@ let print_python_binding name { args; optargs; ret; may_set_error } =
| BytesIn (n, _) ->
pr " Py_buffer %s;\n" n
| BytesOut (n, count) ->
- pr " char *%s;\n" n;
+ pr " char *%s = NULL;\n" n;
pr " Py_ssize_t %s;\n" count
| BytesPersistIn (n, _)
| BytesPersistOut (n, _) ->
@@ -279,11 +279,10 @@ let print_python_binding name { args; optargs; ret; may_set_error } =
n;
pr " struct py_aio_buffer *%s_buf;\n" n
| Closure { cbname } ->
- pr " struct user_data *%s_user_data = alloc_user_data ();\n" cbname;
- pr " if (%s_user_data == NULL) goto out;\n" cbname;
+ pr " struct user_data *%s_user_data = NULL;\n" cbname;
+ pr " PyObject *py_%s_fn;\n" cbname;
pr " nbd_%s_callback %s = { .callback = %s_wrapper,\n"
cbname cbname cbname;
- pr " .user_data = %s_user_data,\n" cbname;
pr " .free = free_user_data };\n"
| Enum (n, _) -> pr " int %s;\n" n
| Flags (n, _) ->
@@ -316,11 +315,10 @@ let print_python_binding name { args; optargs; ret; may_set_error } =
List.iter (
function
| OClosure { cbname } ->
- pr " struct user_data *%s_user_data = alloc_user_data ();\n" cbname;
- pr " if (%s_user_data == NULL) goto out;\n" cbname;
+ pr " struct user_data *%s_user_data = NULL;\n" cbname;
+ pr " PyObject *py_%s_fn;\n" cbname;
pr " nbd_%s_callback %s = { .callback = %s_wrapper,\n"
cbname cbname cbname;
- pr " .user_data = %s_user_data,\n" cbname;
pr " .free = free_user_data };\n"
| OFlags (n, _) ->
pr " uint32_t %s_u32;\n" n;
@@ -329,7 +327,7 @@ let print_python_binding name { args; optargs; ret; may_set_error } =
pr "\n";
(* Parse the Python parameters. *)
- pr " if (!PyArg_ParseTuple (args, (char *) \"O\"";
+ pr " if (!PyArg_ParseTuple (args, \"O\"";
List.iter (
function
| Bool n -> pr " \"p\""
@@ -364,7 +362,7 @@ let print_python_binding name { args; optargs; ret; may_set_error } =
| BytesIn (n, _) | BytesPersistIn (n, _)
| BytesPersistOut (n, _) -> pr ", &%s" n
| BytesOut (_, count) -> pr ", &%s" count
- | Closure { cbname } -> pr ", &%s_user_data->fn" cbname
+ | Closure { cbname } -> pr ", &py_%s_fn" cbname
| Enum (n, _) -> pr ", &%s" n
| Flags (n, _) -> pr ", &%s" n
| Fd n | Int n -> pr ", &%s" n
@@ -379,30 +377,57 @@ let print_python_binding name { args; optargs; ret; may_set_error } =
) args;
List.iter (
function
- | OClosure { cbname } -> pr ", &%s_user_data->fn" cbname
+ | OClosure { cbname } -> pr ", &py_%s_fn" cbname
| OFlags (n, _) -> pr ", &%s" n
) optargs;
pr "))\n";
pr " goto out;\n";
pr " h = get_handle (py_h);\n";
+ pr " if (!h) goto out;\n";
+ List.iter (
+ function
+ | OClosure { cbname } ->
+ pr " %s.user_data = %s_user_data = alloc_user_data ();\n" cbname cbname;
+ pr " if (%s_user_data == NULL) goto out;\n" cbname;
+ pr " if (py_%s_fn != Py_None) {\n" cbname;
+ pr " if (!PyCallable_Check (py_%s_fn)) {\n" cbname;
+ pr " PyErr_SetString (PyExc_TypeError,\n";
+ pr " \"callback parameter %s is not callable\");\n" cbname;
+ pr " goto out;\n";
+ pr " }\n";
+ pr " /* Increment refcount since pointer may be saved by libnbd. */\n";
+ pr " Py_INCREF (py_%s_fn);\n" cbname;
+ pr " %s_user_data->fn = py_%s_fn;\n" cbname cbname;
+ pr " }\n";
+ pr " else\n";
+ pr " %s.callback = NULL; /* we're not going to call it */\n" cbname
+ | OFlags (n, _) -> pr " %s_u32 = %s;\n" n n
+ ) optargs;
List.iter (
function
| Bool _ -> ()
| BytesIn _ -> ()
| BytesOut (n, count) ->
- pr " %s = malloc (%s);\n" n count
+ pr " %s = malloc (%s);\n" n count;
+ pr " if (%s == NULL) { PyErr_NoMemory (); goto out; }\n" n
| BytesPersistIn (n, _) | BytesPersistOut (n, _) ->
- pr " %s_buf = nbd_internal_py_get_aio_buffer (%s);\n" n n
+ pr " %s_buf = nbd_internal_py_get_aio_buffer (%s);\n" n n;
+ pr " if (!%s_buf) goto out;\n" n;
+ pr " /* Increment refcount since buffer may be saved by libnbd. */\n";
+ pr " Py_INCREF (%s);\n" n;
+ pr " completion_user_data->buf = %s;\n" n
| Closure { cbname } ->
- pr " /* Increment refcount since pointer may be saved by libnbd. */\n";
- pr " if (!PyCallable_Check (%s_user_data->fn)) {\n" cbname;
+ pr " %s.user_data = %s_user_data = alloc_user_data ();\n" cbname cbname;
+ pr " if (%s_user_data == NULL) goto out;\n" cbname;
+ pr " if (!PyCallable_Check (py_%s_fn)) {\n" cbname;
pr " PyErr_SetString (PyExc_TypeError,\n";
pr " \"callback parameter %s is not callable\");\n" cbname;
- pr " %s_user_data->fn = NULL;\n" cbname;
pr " goto out;\n";
pr " }\n";
- pr " Py_INCREF (%s_user_data->fn);\n" cbname
+ pr " /* Increment refcount since pointer may be saved by libnbd. */\n";
+ pr " Py_INCREF (py_%s_fn);\n" cbname;
+ pr " %s_user_data->fn = py_%s_fn;\n" cbname cbname
| Enum _ -> ()
| Flags (n, _) -> pr " %s_u32 = %s;\n" n n
| Fd _ | Int _ -> ()
@@ -420,37 +445,6 @@ let print_python_binding name { args; optargs; ret; may_set_error } =
| UInt32 n -> pr " %s_u32 = %s;\n" n n
| UInt64 n -> pr " %s_u64 = %s;\n" n n
) args;
- List.iter (
- function
- | OClosure { cbname } ->
- pr " if (%s_user_data->fn != Py_None) {\n" cbname;
- pr " /* Increment refcount since pointer may be saved by libnbd. */\n";
- pr " if (!PyCallable_Check (%s_user_data->fn)) {\n" cbname;
- pr " PyErr_SetString (PyExc_TypeError,\n";
- pr " \"callback parameter %s is not callable\");\n" cbname;
- pr " %s_user_data->fn = NULL;\n" cbname;
- pr " goto out;\n";
- pr " }\n";
- pr " Py_INCREF (%s_user_data->fn);\n" cbname;
- pr " }\n";
- pr " else\n";
- pr " %s.callback = NULL; /* we're not going to call it */\n" cbname
- | OFlags (n, _) -> pr " %s_u32 = %s;\n" n n
- ) optargs;
-
- (* If there is a BytesPersistIn/Out parameter then we need to
- * increment the refcount and save the pointer into
- * completion_callback.user_data so we can decrement the
- * refcount on command completion.
- *)
- List.iter (
- function
- | BytesPersistIn (n, _) | BytesPersistOut (n, _) ->
- pr " /* Increment refcount since buffer may be saved by libnbd. */\n";
- pr " Py_INCREF (%s);\n" n;
- pr " completion_user_data->buf = %s;\n" n;
- | _ -> ()
- ) args;
pr "\n";
(* Call the underlying C function. *)
@@ -547,9 +541,10 @@ let print_python_binding name { args; optargs; ret; may_set_error } =
function
| Bool _ -> ()
| BytesIn (n, _) -> pr " PyBuffer_Release (&%s);\n" n
- | BytesPersistIn _ | BytesOut _ | BytesPersistOut _ -> ()
+ | BytesOut (n, _) -> pr " free (%s);\n" n
+ | BytesPersistIn _ | BytesPersistOut _ -> ()
| Closure { cbname } ->
- pr " if (%s_user_data) free_user_data (%s_user_data);\n" cbname cbname
+ pr " free_user_data (%s_user_data);\n" cbname
| Enum _ -> ()
| Flags _ -> ()
| Fd _ | Int _ -> ()
@@ -566,7 +561,7 @@ let print_python_binding name { args; optargs; ret; may_set_error } =
List.iter (
function
| OClosure { cbname } ->
- pr " if (%s_user_data) free_user_data (%s_user_data);\n" cbname cbname
+ pr " free_user_data (%s_user_data);\n" cbname
| OFlags _ -> ()
) optargs;
pr " return py_ret;\n";
@@ -613,9 +608,11 @@ let generate_python_methods_c () =
pr "{\n";
pr " struct user_data *data = user_data;\n";
pr "\n";
- pr " Py_XDECREF (data->fn);\n";
- pr " Py_XDECREF (data->buf);\n";
- pr " free (data);\n";
+ pr " if (data) {\n";
+ pr " Py_XDECREF (data->fn);\n";
+ pr " Py_XDECREF (data->buf);\n";
+ pr " free (data);\n";
+ pr " }\n";
pr "}\n";
pr "\n";
diff --git a/python/handle.c b/python/handle.c
index 408a140..12c9c9c 100644
--- a/python/handle.c
+++ b/python/handle.c
@@ -1,5 +1,5 @@
/* NBD client library in userspace
- * Copyright (C) 2013-2019 Red Hat Inc.
+ * Copyright (C) 2013-2020 Red Hat Inc.
*
* This library is free software; you can redistribute it and/or
* modify it under the terms of the GNU Lesser General Public
@@ -91,7 +91,8 @@ free_aio_buffer (PyObject *capsule)
{
struct py_aio_buffer *buf = PyCapsule_GetPointer (capsule, aio_buffer_name);
- free (buf->data);
+ if (buf)
+ free (buf->data);
free (buf);
}
diff --git a/python/utils.c b/python/utils.c
index e0929dc..0e3164c 100644
--- a/python/utils.c
+++ b/python/utils.c
@@ -60,15 +60,24 @@ nbd_internal_py_get_string_list (PyObject *obj)
for (i = 0; i < len; ++i) {
PyObject *bytes = PyUnicode_AsUTF8String (PyList_GetItem (obj, i));
+ if (!bytes)
+ goto err;
r[i] = strdup (PyBytes_AS_STRING (bytes));
+ Py_DECREF (bytes);
if (r[i] == NULL) {
PyErr_NoMemory ();
- return NULL;
+ goto err;
}
}
r[len] = NULL;
return r;
+
+ err:
+ while (i--)
+ free (r[i]);
+ free (r);
+ return NULL;
}
void
--
2.28.0
3 years, 8 months
[libnbd PATCH] python: Plug some memory leaks on error paths
by Eric Blake
Inspection of the generated code showed several places where we did
not release references on all error paths. In particular, switching
things to always jump to an out: label, even when the underlying C
function cannot fail, makes it easier to clean up when the user passes
wrong types to a function call.
One of the easiest triggers without this patch was this one-liner:
$ nbdsh -c 'h.block_status(512, 0, 1)'
which fails due to '1' not being callable, but leaked malloc'd memory
in the process.
---
generator/Python.ml | 55 +++++++++++++++++++++++++++++----------------
1 file changed, 36 insertions(+), 19 deletions(-)
diff --git a/generator/Python.ml b/generator/Python.ml
index 4a96cf6..9a22f9e 100644
--- a/generator/Python.ml
+++ b/generator/Python.ml
@@ -177,6 +177,7 @@ let print_python_closure_wrapper { cbname; cbargs } =
pr " Py_DECREF (py_%s_modname);\n" n;
pr " if (!py_%s_mod) { PyErr_PrintEx (0); return -1; }\n" n;
pr " PyObject *py_%s = PyObject_CallMethod (py_%s_mod, \"c_int\", \"i\", *%s);\n" n n n;
+ pr " Py_DECREF (py_%s_mod);\n" n;
pr " if (!py_%s) { PyErr_PrintEx (0); return -1; }\n" n;
| CBString _
| CBUInt _
@@ -263,7 +264,7 @@ let print_python_binding name { args; optargs; ret; may_set_error } =
pr " PyObject *py_h;\n";
pr " struct nbd_handle *h;\n";
pr " %s ret;\n" (C.type_of_ret ret);
- pr " PyObject *py_ret;\n";
+ pr " PyObject *py_ret = NULL;\n";
List.iter (
function
| Bool n -> pr " int %s;\n" n
@@ -279,7 +280,7 @@ let print_python_binding name { args; optargs; ret; may_set_error } =
pr " struct py_aio_buffer *%s_buf;\n" n
| Closure { cbname } ->
pr " struct user_data *%s_user_data = alloc_user_data ();\n" cbname;
- pr " if (%s_user_data == NULL) return NULL;\n" cbname;
+ pr " if (%s_user_data == NULL) goto out;\n" cbname;
pr " nbd_%s_callback %s = { .callback = %s_wrapper,\n"
cbname cbname cbname;
pr " .user_data = %s_user_data,\n" cbname;
@@ -316,7 +317,7 @@ let print_python_binding name { args; optargs; ret; may_set_error } =
function
| OClosure { cbname } ->
pr " struct user_data *%s_user_data = alloc_user_data ();\n" cbname;
- pr " if (%s_user_data == NULL) return NULL;\n" cbname;
+ pr " if (%s_user_data == NULL) goto out;\n" cbname;
pr " nbd_%s_callback %s = { .callback = %s_wrapper,\n"
cbname cbname cbname;
pr " .user_data = %s_user_data,\n" cbname;
@@ -382,7 +383,7 @@ let print_python_binding name { args; optargs; ret; may_set_error } =
| OFlags (n, _) -> pr ", &%s" n
) optargs;
pr "))\n";
- pr " return NULL;\n";
+ pr " goto out;\n";
pr " h = get_handle (py_h);\n";
List.iter (
@@ -395,12 +396,13 @@ let print_python_binding name { args; optargs; ret; may_set_error } =
pr " %s_buf = nbd_internal_py_get_aio_buffer (%s);\n" n n
| Closure { cbname } ->
pr " /* Increment refcount since pointer may be saved by libnbd. */\n";
- pr " Py_INCREF (%s_user_data->fn);\n" cbname;
pr " if (!PyCallable_Check (%s_user_data->fn)) {\n" cbname;
pr " PyErr_SetString (PyExc_TypeError,\n";
pr " \"callback parameter %s is not callable\");\n" cbname;
- pr " return NULL;\n";
- pr " }\n"
+ pr " %s_user_data->fn = NULL;\n" cbname;
+ pr " goto out;\n";
+ pr " }\n";
+ pr " Py_INCREF (%s_user_data->fn);\n" cbname
| Enum _ -> ()
| Flags (n, _) -> pr " %s_u32 = %s;\n" n n
| Fd _ | Int _ -> ()
@@ -413,7 +415,7 @@ let print_python_binding name { args; optargs; ret; may_set_error } =
| String _ -> ()
| StringList n ->
pr " %s = nbd_internal_py_get_string_list (py_%s);\n" n n;
- pr " if (!%s) { py_ret = NULL; goto out; }\n" n
+ pr " if (!%s) goto out;\n" n
| UInt _ -> ()
| UInt32 n -> pr " %s_u32 = %s;\n" n n
| UInt64 n -> pr " %s_u64 = %s;\n" n n
@@ -423,12 +425,13 @@ let print_python_binding name { args; optargs; ret; may_set_error } =
| OClosure { cbname } ->
pr " if (%s_user_data->fn != Py_None) {\n" cbname;
pr " /* Increment refcount since pointer may be saved by libnbd. */\n";
- pr " Py_INCREF (%s_user_data->fn);\n" cbname;
pr " if (!PyCallable_Check (%s_user_data->fn)) {\n" cbname;
pr " PyErr_SetString (PyExc_TypeError,\n";
pr " \"callback parameter %s is not callable\");\n" cbname;
- pr " return NULL;\n";
+ pr " %s_user_data->fn = NULL;\n" cbname;
+ pr " goto out;\n";
pr " }\n";
+ pr " Py_INCREF (%s_user_data->fn);\n" cbname;
pr " }\n";
pr " else\n";
pr " %s.callback = NULL; /* we're not going to call it */\n" cbname
@@ -447,7 +450,8 @@ let print_python_binding name { args; optargs; ret; may_set_error } =
pr " Py_INCREF (%s);\n" n;
pr " completion_user_data->buf = %s;\n" n;
| _ -> ()
- ) args;
+ ) args;
+ pr "\n";
(* Call the underlying C function. *)
pr " ret = nbd_%s (h" name;
@@ -477,11 +481,20 @@ let print_python_binding name { args; optargs; ret; may_set_error } =
| OFlags (n, _) -> pr ", %s_u32" n
) optargs;
pr ");\n";
+ List.iter (
+ function
+ | Closure { cbname } -> pr " %s_user_data = NULL;\n" cbname
+ | _ -> ()
+ ) args;
+ List.iter (
+ function
+ | OClosure { cbname } -> pr " %s_user_data = NULL;\n" cbname
+ | _ -> ()
+ ) optargs;
if may_set_error then (
pr " if (ret == %s) {\n"
(match C.errcode_of_ret ret with Some s -> s | None -> assert false);
pr " raise_exception ();\n";
- pr " py_ret = NULL;\n";
pr " goto out;\n";
pr " }\n"
);
@@ -529,14 +542,14 @@ let print_python_binding name { args; optargs; ret; may_set_error } =
);
pr "\n";
- if may_set_error then
- pr " out:\n";
+ pr " out:\n";
List.iter (
function
| Bool _ -> ()
| BytesIn (n, _) -> pr " PyBuffer_Release (&%s);\n" n
| BytesPersistIn _ | BytesOut _ | BytesPersistOut _ -> ()
- | Closure _ -> ()
+ | Closure { cbname } ->
+ pr " if (%s_user_data) free_user_data (%s_user_data);\n" cbname cbname
| Enum _ -> ()
| Flags _ -> ()
| Fd _ | Int _ -> ()
@@ -550,6 +563,12 @@ let print_python_binding name { args; optargs; ret; may_set_error } =
| UInt32 _ -> ()
| UInt64 _ -> ()
) args;
+ List.iter (
+ function
+ | OClosure { cbname } ->
+ pr " if (%s_user_data) free_user_data (%s_user_data);\n" cbname cbname
+ | OFlags _ -> ()
+ ) optargs;
pr " return py_ret;\n";
pr "}\n";
pr "\n"
@@ -594,10 +613,8 @@ let generate_python_methods_c () =
pr "{\n";
pr " struct user_data *data = user_data;\n";
pr "\n";
- pr " if (data->fn != NULL)\n";
- pr " Py_DECREF (data->fn);\n";
- pr " if (data->buf != NULL)\n";
- pr " Py_DECREF (data->buf);\n";
+ pr " Py_XDECREF (data->fn);\n";
+ pr " Py_XDECREF (data->buf);\n";
pr " free (data);\n";
pr "}\n";
pr "\n";
--
2.28.0
3 years, 8 months
[libnbd PATCH] hack for testing python closure leaks: NOT FOR COMMIT
by Eric Blake
Just posting this for posterity, on something I used to track down
memory leaks related to python closures.
---
generator/Python.ml | 19 +++++++++++++++++++
1 file changed, 19 insertions(+)
diff --git a/generator/Python.ml b/generator/Python.ml
index 4a96cf6..2ff17dd 100644
--- a/generator/Python.ml
+++ b/generator/Python.ml
@@ -578,9 +578,27 @@ let generate_python_methods_c () =
pr " PyObject *buf; /* Optional pointer to persistent buffer. */\n";
pr "};\n";
pr "\n";
+ pr "static size_t alloc_count;\n";
+ pr "void\n";
+ pr "dump_alloc_count (void)\n";
+ pr "{\n";
+ pr " char *cmd;\n";
+ pr " if (!alloc_count) return;\n";
+ pr " asprintf (&cmd, \"{ echo ' *** alloc_count:%%zu'; cat /proc/%%d/cmdline; } > /dev/tty\",\n";
+ pr " alloc_count, getpid ());\n";
+ pr " system (cmd);\n";
+ pr " free (cmd);\n";
+ pr "}\n";
+ pr "\n";
pr "static struct user_data *\n";
pr "alloc_user_data (void)\n";
pr "{\n";
+ pr " static bool init;\n";
+ pr " if (!init) {\n";
+ pr " init = true;\n";
+ pr " atexit (dump_alloc_count);\n";
+ pr " }\n";
+ pr " alloc_count++;\n";
pr " struct user_data *data = calloc (1, sizeof *data);\n";
pr " if (data == NULL) {\n";
pr " PyErr_NoMemory ();\n";
@@ -594,6 +612,7 @@ let generate_python_methods_c () =
pr "{\n";
pr " struct user_data *data = user_data;\n";
pr "\n";
+ pr " alloc_count--;\n";
pr " if (data->fn != NULL)\n";
pr " Py_DECREF (data->fn);\n";
pr " if (data->buf != NULL)\n";
--
2.28.0
3 years, 8 months