[Libguestfs] [libnbd PATCH 1/2] generator: Refactor handling of closures in unlocked functions
Eric Blake
eblake at redhat.com
Mon Sep 7 21:46:39 UTC 2020
We have a memory leak when a function with a closure exits early prior
to registering that closure through some path that will guarantee
cleanup. The easiest way to fix it is to guarantee that once a
closure is passed into a public API, it will be cleaned regardless of
whether that API succeeds or fails. But to avoid cleaning the closure
more than once, we need to refactor our internal handling, in order to
track when a closure has been handed off for later cleaning. The
easiest way to do this is to pass closures by reference to all
internal functions, so that helpers in the call stack can modify the
incoming pointer rather than their own copy.
This patch is just preparatory, with no semantic change. The public
API still passes closures by copy, but the generator then operates on
the address of that closure through all internal nbd_unlocked_*
functions, rather than making further copies through each additional
function call.
---
generator/C.ml | 35 ++++++++++++-----------
generator/C.mli | 1 +
lib/debug.c | 6 ++--
lib/opt.c | 26 ++++++++---------
lib/rw.c | 76 ++++++++++++++++++++++++-------------------------
5 files changed, 73 insertions(+), 71 deletions(-)
diff --git a/generator/C.ml b/generator/C.ml
index deb77fa..280b319 100644
--- a/generator/C.ml
+++ b/generator/C.ml
@@ -99,16 +99,17 @@ let rec name_of_arg = function
| UInt64 n -> [n]
let rec print_arg_list ?(wrap = false) ?maxcol ?handle ?types ?(parens = true)
+ ?(closure_mark) args optargs =
+ if parens then pr "(";
+ if wrap then
+ pr_wrap ?maxcol ','
+ (fun () -> print_arg_list' ?handle ?types ?closure_mark args optargs)
+ else
+ print_arg_list' ?handle ?types ?closure_mark args optargs;
+ if parens then pr ")"
+
+and print_arg_list' ?(handle = false) ?(types = true) ?(closure_mark = "")
args optargs =
- if parens then pr "(";
- if wrap then
- pr_wrap ?maxcol ','
- (fun () -> print_arg_list' ?handle ?types args optargs)
- else
- print_arg_list' ?handle ?types args optargs;
- if parens then pr ")"
-
-and print_arg_list' ?(handle = false) ?(types = true) args optargs =
let comma = ref false in
if handle then (
comma := true;
@@ -137,7 +138,7 @@ and print_arg_list' ?(handle = false) ?(types = true) args optargs =
pr "%s" len
| Closure { cbname; cbargs } ->
if types then pr "nbd_%s_callback " cbname;
- pr "%s_callback" cbname
+ pr "%s%s_callback" closure_mark cbname
| Enum (n, _) ->
if types then pr "int ";
pr "%s" n
@@ -179,19 +180,19 @@ and print_arg_list' ?(handle = false) ?(types = true) args optargs =
match optarg with
| OClosure { cbname; cbargs } ->
if types then pr "nbd_%s_callback " cbname;
- pr "%s_callback" cbname
+ pr "%s%s_callback" closure_mark cbname
| OFlags (n, _) ->
if types then pr "uint32_t ";
pr "%s" n
) optargs
-let print_call ?wrap ?maxcol name args optargs ret =
+let print_call ?wrap ?maxcol ?closure_mark name args optargs ret =
pr "%s nbd_%s " (type_of_ret ret) name;
- print_arg_list ~handle:true ?wrap ?maxcol args optargs
+ print_arg_list ~handle:true ?wrap ?maxcol ?closure_mark args optargs
-let print_extern ?wrap name args optargs ret =
+let print_extern ?wrap ?closure_mark name args optargs ret =
pr "extern ";
- print_call ?wrap name args optargs ret;
+ print_call ?wrap ?closure_mark name args optargs ret;
pr ";\n"
let rec print_cbarg_list ?(wrap = false) ?maxcol ?types ?(parens = true)
@@ -385,7 +386,7 @@ let generate_lib_unlocked_h () =
pr "\n";
List.iter (
fun (name, { args; optargs; ret }) ->
- print_extern ~wrap:true ("unlocked_" ^ name) args optargs ret
+ print_extern ~wrap:true ~closure_mark:"*" ("unlocked_" ^ name) args optargs ret
) handle_calls;
pr "\n";
pr "#endif /* LIBNBD_UNLOCKED_H */\n"
@@ -543,7 +544,7 @@ let generate_lib_api_c () =
(* Make the call. *)
pr " ret = nbd_unlocked_%s " name;
- print_arg_list ~wrap:true ~types:false ~handle:true args optargs;
+ print_arg_list ~wrap:true ~types:false ~handle:true ~closure_mark:"&" args optargs;
pr ";\n";
pr "\n";
if !need_out_label then
diff --git a/generator/C.mli b/generator/C.mli
index 4f71453..a7e0412 100644
--- a/generator/C.mli
+++ b/generator/C.mli
@@ -27,6 +27,7 @@ val generate_docs_api_flag_links_pod : unit -> unit
val generate_docs_nbd_pod : string -> API.call -> unit -> unit
val print_arg_list : ?wrap:bool -> ?maxcol:int ->
?handle:bool -> ?types:bool -> ?parens:bool ->
+ ?closure_mark:string ->
API.arg list -> API.optarg list -> unit
val print_cbarg_list : ?wrap:bool -> ?maxcol:int ->
?types:bool -> ?parens:bool ->
diff --git a/lib/debug.c b/lib/debug.c
index fbedbc9..1b503d9 100644
--- a/lib/debug.c
+++ b/lib/debug.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
@@ -48,12 +48,12 @@ nbd_unlocked_clear_debug_callback (struct nbd_handle *h)
int
nbd_unlocked_set_debug_callback (struct nbd_handle *h,
- nbd_debug_callback debug_callback)
+ nbd_debug_callback *debug_callback)
{
/* This can't fail at the moment - see implementation above. */
nbd_unlocked_clear_debug_callback (h);
- h->debug_callback = debug_callback;
+ h->debug_callback = *debug_callback;
return 0;
}
diff --git a/lib/opt.c b/lib/opt.c
index cc0c145..003ecf8 100644
--- a/lib/opt.c
+++ b/lib/opt.c
@@ -74,7 +74,7 @@ nbd_unlocked_opt_go (struct nbd_handle *h)
{
int err;
nbd_completion_callback c = { .callback = go_complete, .user_data = &err };
- int r = nbd_unlocked_aio_opt_go (h, c);
+ int r = nbd_unlocked_aio_opt_go (h, &c);
if (r == -1)
return r;
@@ -96,7 +96,7 @@ nbd_unlocked_opt_info (struct nbd_handle *h)
{
int err;
nbd_completion_callback c = { .callback = go_complete, .user_data = &err };
- int r = nbd_unlocked_aio_opt_info (h, c);
+ int r = nbd_unlocked_aio_opt_info (h, &c);
if (r == -1)
return r;
@@ -147,13 +147,13 @@ list_complete (void *opaque, int *err)
/* Issue NBD_OPT_LIST and wait for the reply. */
int
-nbd_unlocked_opt_list (struct nbd_handle *h, nbd_list_callback list)
+nbd_unlocked_opt_list (struct nbd_handle *h, nbd_list_callback *list)
{
- struct list_helper s = { .list = list };
+ struct list_helper s = { .list = *list };
nbd_list_callback l = { .callback = list_visitor, .user_data = &s };
nbd_completion_callback c = { .callback = list_complete, .user_data = &s };
- if (nbd_unlocked_aio_opt_list (h, l, c) == -1)
+ if (nbd_unlocked_aio_opt_list (h, &l, &c) == -1)
return -1;
if (wait_for_option (h) == -1)
@@ -168,10 +168,10 @@ nbd_unlocked_opt_list (struct nbd_handle *h, nbd_list_callback list)
/* Issue NBD_OPT_GO (or NBD_OPT_EXPORT_NAME) without waiting. */
int
nbd_unlocked_aio_opt_go (struct nbd_handle *h,
- nbd_completion_callback complete)
+ nbd_completion_callback *complete)
{
h->opt_current = NBD_OPT_GO;
- h->opt_cb.completion = complete;
+ h->opt_cb.completion = *complete;
if (nbd_internal_run (h, cmd_issue) == -1)
debug (h, "option queued, ignoring state machine failure");
@@ -181,7 +181,7 @@ nbd_unlocked_aio_opt_go (struct nbd_handle *h,
/* Issue NBD_OPT_INFO without waiting. */
int
nbd_unlocked_aio_opt_info (struct nbd_handle *h,
- nbd_completion_callback complete)
+ nbd_completion_callback *complete)
{
if ((h->gflags & LIBNBD_HANDSHAKE_FLAG_FIXED_NEWSTYLE) == 0) {
set_error (ENOTSUP, "server is not using fixed newstyle protocol");
@@ -189,7 +189,7 @@ nbd_unlocked_aio_opt_info (struct nbd_handle *h,
}
h->opt_current = NBD_OPT_INFO;
- h->opt_cb.completion = complete;
+ h->opt_cb.completion = *complete;
if (nbd_internal_run (h, cmd_issue) == -1)
debug (h, "option queued, ignoring state machine failure");
@@ -209,8 +209,8 @@ nbd_unlocked_aio_opt_abort (struct nbd_handle *h)
/* Issue NBD_OPT_LIST without waiting. */
int
-nbd_unlocked_aio_opt_list (struct nbd_handle *h, nbd_list_callback list,
- nbd_completion_callback complete)
+nbd_unlocked_aio_opt_list (struct nbd_handle *h, nbd_list_callback *list,
+ nbd_completion_callback *complete)
{
if ((h->gflags & LIBNBD_HANDSHAKE_FLAG_FIXED_NEWSTYLE) == 0) {
set_error (ENOTSUP, "server is not using fixed newstyle protocol");
@@ -218,8 +218,8 @@ nbd_unlocked_aio_opt_list (struct nbd_handle *h, nbd_list_callback list,
}
assert (CALLBACK_IS_NULL (h->opt_cb.fn.list));
- h->opt_cb.fn.list = list;
- h->opt_cb.completion = complete;
+ h->opt_cb.fn.list = *list;
+ h->opt_cb.completion = *complete;
h->opt_current = NBD_OPT_LIST;
if (nbd_internal_run (h, cmd_issue) == -1)
debug (h, "option queued, ignoring state machine failure");
diff --git a/lib/rw.c b/lib/rw.c
index 8b736d3..9f2909b 100644
--- a/lib/rw.c
+++ b/lib/rw.c
@@ -48,9 +48,9 @@ nbd_unlocked_pread (struct nbd_handle *h, void *buf,
size_t count, uint64_t offset, uint32_t flags)
{
int64_t cookie;
+ nbd_completion_callback c = NBD_NULL_COMPLETION;
- cookie = nbd_unlocked_aio_pread (h, buf, count, offset,
- NBD_NULL_COMPLETION, flags);
+ cookie = nbd_unlocked_aio_pread (h, buf, count, offset, &c, flags);
if (cookie == -1)
return -1;
@@ -61,15 +61,14 @@ nbd_unlocked_pread (struct nbd_handle *h, void *buf,
int
nbd_unlocked_pread_structured (struct nbd_handle *h, void *buf,
size_t count, uint64_t offset,
- nbd_chunk_callback chunk,
+ nbd_chunk_callback *chunk,
uint32_t flags)
{
int64_t cookie;
+ nbd_completion_callback c = NBD_NULL_COMPLETION;
cookie = nbd_unlocked_aio_pread_structured (h, buf, count, offset,
- chunk,
- NBD_NULL_COMPLETION,
- flags);
+ chunk, &c, flags);
if (cookie == -1)
return -1;
@@ -82,9 +81,9 @@ nbd_unlocked_pwrite (struct nbd_handle *h, const void *buf,
size_t count, uint64_t offset, uint32_t flags)
{
int64_t cookie;
+ nbd_completion_callback c = NBD_NULL_COMPLETION;
- cookie = nbd_unlocked_aio_pwrite (h, buf, count, offset,
- NBD_NULL_COMPLETION, flags);
+ cookie = nbd_unlocked_aio_pwrite (h, buf, count, offset, &c, flags);
if (cookie == -1)
return -1;
@@ -96,8 +95,9 @@ int
nbd_unlocked_flush (struct nbd_handle *h, uint32_t flags)
{
int64_t cookie;
+ nbd_completion_callback c = NBD_NULL_COMPLETION;
- cookie = nbd_unlocked_aio_flush (h, NBD_NULL_COMPLETION, flags);
+ cookie = nbd_unlocked_aio_flush (h, &c, flags);
if (cookie == -1)
return -1;
@@ -110,9 +110,9 @@ nbd_unlocked_trim (struct nbd_handle *h,
uint64_t count, uint64_t offset, uint32_t flags)
{
int64_t cookie;
+ nbd_completion_callback c = NBD_NULL_COMPLETION;
- cookie = nbd_unlocked_aio_trim (h, count, offset,
- NBD_NULL_COMPLETION, flags);
+ cookie = nbd_unlocked_aio_trim (h, count, offset, &c, flags);
if (cookie == -1)
return -1;
@@ -125,9 +125,9 @@ nbd_unlocked_cache (struct nbd_handle *h,
uint64_t count, uint64_t offset, uint32_t flags)
{
int64_t cookie;
+ nbd_completion_callback c = NBD_NULL_COMPLETION;
- cookie = nbd_unlocked_aio_cache (h, count, offset,
- NBD_NULL_COMPLETION, flags);
+ cookie = nbd_unlocked_aio_cache (h, count, offset, &c, flags);
if (cookie == -1)
return -1;
@@ -140,9 +140,9 @@ nbd_unlocked_zero (struct nbd_handle *h,
uint64_t count, uint64_t offset, uint32_t flags)
{
int64_t cookie;
+ nbd_completion_callback c = NBD_NULL_COMPLETION;
- cookie = nbd_unlocked_aio_zero (h, count, offset,
- NBD_NULL_COMPLETION, flags);
+ cookie = nbd_unlocked_aio_zero (h, count, offset, &c, flags);
if (cookie == -1)
return -1;
@@ -153,13 +153,13 @@ nbd_unlocked_zero (struct nbd_handle *h,
int
nbd_unlocked_block_status (struct nbd_handle *h,
uint64_t count, uint64_t offset,
- nbd_extent_callback extent,
+ nbd_extent_callback *extent,
uint32_t flags)
{
int64_t cookie;
+ nbd_completion_callback c = NBD_NULL_COMPLETION;
- cookie = nbd_unlocked_aio_block_status (h, count, offset, extent,
- NBD_NULL_COMPLETION, flags);
+ cookie = nbd_unlocked_aio_block_status (h, count, offset, extent, &c, flags);
if (cookie == -1)
return -1;
@@ -262,10 +262,10 @@ nbd_internal_command_common (struct nbd_handle *h,
int64_t
nbd_unlocked_aio_pread (struct nbd_handle *h, void *buf,
size_t count, uint64_t offset,
- nbd_completion_callback completion,
+ nbd_completion_callback *completion,
uint32_t flags)
{
- struct command_cb cb = { .completion = completion };
+ struct command_cb cb = { .completion = *completion };
/* We could silently accept flag DF, but it really only makes sense
* with callbacks, because otherwise there is no observable change
@@ -283,12 +283,12 @@ nbd_unlocked_aio_pread (struct nbd_handle *h, void *buf,
int64_t
nbd_unlocked_aio_pread_structured (struct nbd_handle *h, void *buf,
size_t count, uint64_t offset,
- nbd_chunk_callback chunk,
- nbd_completion_callback completion,
+ nbd_chunk_callback *chunk,
+ nbd_completion_callback *completion,
uint32_t flags)
{
- struct command_cb cb = { .fn.chunk = chunk,
- .completion = completion };
+ struct command_cb cb = { .fn.chunk = *chunk,
+ .completion = *completion };
if ((flags & ~LIBNBD_CMD_FLAG_DF) != 0) {
set_error (EINVAL, "invalid flag: %" PRIu32, flags);
@@ -308,10 +308,10 @@ nbd_unlocked_aio_pread_structured (struct nbd_handle *h, void *buf,
int64_t
nbd_unlocked_aio_pwrite (struct nbd_handle *h, const void *buf,
size_t count, uint64_t offset,
- nbd_completion_callback completion,
+ nbd_completion_callback *completion,
uint32_t flags)
{
- struct command_cb cb = { .completion = completion };
+ struct command_cb cb = { .completion = *completion };
if (nbd_unlocked_is_read_only (h) == 1) {
set_error (EPERM, "server does not support write operations");
@@ -335,10 +335,10 @@ nbd_unlocked_aio_pwrite (struct nbd_handle *h, const void *buf,
int64_t
nbd_unlocked_aio_flush (struct nbd_handle *h,
- nbd_completion_callback completion,
+ nbd_completion_callback *completion,
uint32_t flags)
{
- struct command_cb cb = { .completion = completion };
+ struct command_cb cb = { .completion = *completion };
if (nbd_unlocked_can_flush (h) != 1) {
set_error (EINVAL, "server does not support flush operations");
@@ -357,10 +357,10 @@ nbd_unlocked_aio_flush (struct nbd_handle *h,
int64_t
nbd_unlocked_aio_trim (struct nbd_handle *h,
uint64_t count, uint64_t offset,
- nbd_completion_callback completion,
+ nbd_completion_callback *completion,
uint32_t flags)
{
- struct command_cb cb = { .completion = completion };
+ struct command_cb cb = { .completion = *completion };
if (nbd_unlocked_can_trim (h) != 1) {
set_error (EINVAL, "server does not support trim operations");
@@ -394,10 +394,10 @@ nbd_unlocked_aio_trim (struct nbd_handle *h,
int64_t
nbd_unlocked_aio_cache (struct nbd_handle *h,
uint64_t count, uint64_t offset,
- nbd_completion_callback completion,
+ nbd_completion_callback *completion,
uint32_t flags)
{
- struct command_cb cb = { .completion = completion };
+ struct command_cb cb = { .completion = *completion };
/* Actually according to the NBD protocol document, servers do exist
* that support NBD_CMD_CACHE but don't advertise the
@@ -420,10 +420,10 @@ nbd_unlocked_aio_cache (struct nbd_handle *h,
int64_t
nbd_unlocked_aio_zero (struct nbd_handle *h,
uint64_t count, uint64_t offset,
- nbd_completion_callback completion,
+ nbd_completion_callback *completion,
uint32_t flags)
{
- struct command_cb cb = { .completion = completion };
+ struct command_cb cb = { .completion = *completion };
if (nbd_unlocked_can_zero (h) != 1) {
set_error (EINVAL, "server does not support zero operations");
@@ -464,12 +464,12 @@ nbd_unlocked_aio_zero (struct nbd_handle *h,
int64_t
nbd_unlocked_aio_block_status (struct nbd_handle *h,
uint64_t count, uint64_t offset,
- nbd_extent_callback extent,
- nbd_completion_callback completion,
+ nbd_extent_callback *extent,
+ nbd_completion_callback *completion,
uint32_t flags)
{
- struct command_cb cb = { .fn.extent = extent,
- .completion = completion };
+ struct command_cb cb = { .fn.extent = *extent,
+ .completion = *completion };
if (!h->structured_replies) {
set_error (ENOTSUP, "server does not support structured replies");
--
2.28.0
More information about the Libguestfs
mailing list