libnbd: When are callbacks freed
by Tage Johansson
Hello,
While writing some tests for the Rust bindings, I discovered a memory
leak with Valgrind due to a completion callback not being freed. More
specificly, the completion callback of `aio_opt_info()` doesn't seem to
be if `aio_opt_info()` failes. In this case, `aio_opt_info()` was called
in the connected state, so it should indeed fail, but it should perhaps
also call the free function associated with the callback.
According to libnbd(3) <https://libguestfs.org/libnbd.3.html>:
> Callback lifetimes
> You can associate an optional free function with callbacks. Libnbd
> will call this function when the callback will not be called again by
> libnbd, including in the case where the API fails.
After studying the code in lib/opt.c, I think that the situation with
synchronous callbacks like `nbd_opt_list()` is even worse. Here the list
callback will not be freed if the internal call to
`nbd_unlocked_aio_opt_list()` failes, but it will be freed if the
completion callback got an error which is returned by `nbd_opt_list()`.
> /* Issue NBD_OPT_LIST and wait for the reply. */
> int
> nbd_unlocked_opt_list (struct nbd_handle *h, nbd_list_callback *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)
Here the list callback has not been freed.
> return -1;
>
> SET_CALLBACK_TO_NULL (*list);
> if (wait_fo_option (h) == -1)
> return -1;
> if (s.err) {
But here I think that the callback has been freed.
> set_error (s.err, "server replied with error to list request");
> return -1;
> }
> return s.count;
> }
The problem is that if `nbd_opt_list()` returns an error, I as a user
cannot know wether I should free the data associated with the list
callback myself or if that has been done already.
Maybe I am just misunderstanding the code or the API, but if not then I
wonder how I should know when I need to free the user data associated
with a callback myself and when that is done by libnbd?
Best regards,
Tage
1 year, 3 months
[PATCH] Add partprobe call in the API
by Philippe Midol-Monnet
Hello
We are using libguestfs with nested partitions: One partition encrypted
with LUKS include a GPT and several partition.
In order to make the nested partition available to the system after the
cryptsetup-open, partprobe need to be run.
I guess it is true for all kind of nested partitions (encrypted or not).
This patch add a partprobe call in the API.
Regards
Philippe
--
*This email and any attachment contains EasyMile’s confidential
information, and must not be modified or circulated without EasyMile’s
prior written consent. It is intended exclusively for their recipient.s. If
you received this message by mistake, please notify us promptly and
immediately delete this email and any of its attachments.*
*As data
controller, EasyMile processes personal data, in compliance with the
European GDPR (EU) 2016/679 of 27 April 2016. To exercise your rights, you
can contact EasyMile at privacy(a)easymile.com, and if necessary, you may
contact the local competent supervisory authority.”*
*Cet e-mail et toute
pièce jointe contiennent des informations confidentielles d'EasyMile et ne
doivent pas être modifiés ou diffusés sans le consentement écrit préalable
d'EasyMile. Ils sont destinés exclusivement à leur destinataire.s. Si vous
avez reçu ce message par erreur, veuillez nous en informer rapidement et
supprimer immédiatement cet e-mail et toutes ses pièces jointes. *
*En
tant que responsable de traitement, EasyMile traite des données
personnelles, conformément au RGPD européen (UE) 2016/679 du 27 avril 2016.
Pour exercer vos droits, vous pouvez contacter EasyMile à
privacy(a)easymile.com <mailto:privacy@easymile.com>, et si nécessaire, vous
pouvez contacter l'autorité de contrôle locale compétente.*
1 year, 3 months
[libnbd PATCH] tests: Test free callback in synchronous nbd_opt_* calls
by Eric Blake
The documentation guarantees that a user's .free callback is reached
exactly once for all synchronous nbd_opt_* functions that take a
callback structure (nbd_opt_list, nbd_opt_list_meta, ...), regardless
of API success or failure, but we weren't actually testing that in the
testsuite. It is quite easy to augment the testsuite C code to prove
that any user's .free callback is reached exactly once, and after all
the .callback calls (if any) have finished. We don't need to change
the matching tests in any of the other language bindings (callbacks in
other languages do not directly expose a .free callback to the user in
the first place, even though the generator heavily relies on the C
.free callback as part of creating the language bindings).
The rest of this commit message is not about the patch proper, but
about understanding why the existing code base works, since it is not
obvious from just reading lib/opt.c. Things to note: our generated
lib/api.c unconditionally uses FREE_CALLBACK() if the corresponding
nbd_unlocked_ call fails for any reason, but this is a no-op if the
code earlier transferred the callback to some other location (a
transfer is marked by SET_CALLBACK_TO_NULL). We trigger transfer
semantics in nbd_unlocked_aio_opt_* only when h->opt_cb is set - at
which point, generator/states-newstyle-opt-*.c guarantees it will call
nbd_internal_free_option() which calls the callback at the completion
of the option, whether by successful movement back to connecting state
(regardless of whether the server replied with success or error), or
by transfer to the DEAD state. Meanwhile, the synchronous calls use a
stack-local completion callback passed to the counterpart aio calls
that kicks off the option sequence because of our use of
{go,list,context}_complete(), and so they have to manually mark the
user's parameter as having been transferred - but again we are
guaranteed that once wait_for_option() returns, the state machine has
already reached a state where our stack-local completion callback was
reached, and therefore the user's free callback has been reached.
Reported-by: Tage Johansson <tage.j.lists(a)posteo.net>
Signed-off-by: Eric Blake <eblake(a)redhat.com>
---
Email sent as an FYI; the patch is already pushed as commit 824de0f1
tests/opt-list-meta.c | 67 ++++++++++++++++++++++++++++++++++++++-----
tests/opt-list.c | 46 ++++++++++++++++++++++++++---
2 files changed, 102 insertions(+), 11 deletions(-)
diff --git a/tests/opt-list-meta.c b/tests/opt-list-meta.c
index cc67fc9b..bb731edd 100644
--- a/tests/opt-list-meta.c
+++ b/tests/opt-list-meta.c
@@ -34,6 +34,7 @@
struct progress {
int count;
bool seen;
+ bool freed;
};
static int
@@ -41,12 +42,29 @@ check (void *user_data, const char *name)
{
struct progress *p = user_data;
+ if (p->freed) {
+ fprintf (stderr, "use after free callback");
+ exit (EXIT_FAILURE);
+ }
+
p->count++;
if (strcmp (name, LIBNBD_CONTEXT_BASE_ALLOCATION) == 0)
p->seen = true;
return 0;
}
+static void
+cb_free (void *user_data)
+{
+ struct progress *p = user_data;
+
+ if (p->freed) {
+ fprintf (stderr, "too many free callbacks");
+ exit (EXIT_FAILURE);
+ }
+ p->freed = true;
+}
+
int
main (int argc, char *argv[])
{
@@ -72,7 +90,8 @@ main (int argc, char *argv[])
p = (struct progress) { .count = 0 };
r = nbd_opt_list_meta_context (nbd,
(nbd_context_callback) { .callback = check,
- .user_data = &p});
+ .user_data = &p,
+ .free = cb_free});
if (r == -1) {
fprintf (stderr, "%s\n", nbd_get_error ());
exit (EXIT_FAILURE);
@@ -85,6 +104,10 @@ main (int argc, char *argv[])
fprintf (stderr, "server did not reply with base:allocation\n");
exit (EXIT_FAILURE);
}
+ if (!p.freed) {
+ fprintf (stderr, "callback not freed by libnbd\n");
+ exit (EXIT_FAILURE);
+ }
max = p.count;
/* Second pass: bogus query has no response. */
@@ -96,7 +119,8 @@ main (int argc, char *argv[])
}
r = nbd_opt_list_meta_context (nbd,
(nbd_context_callback) { .callback = check,
- .user_data = &p});
+ .user_data = &p,
+ .free = cb_free});
if (r == -1) {
fprintf (stderr, "%s\n", nbd_get_error ());
exit (EXIT_FAILURE);
@@ -105,6 +129,10 @@ main (int argc, char *argv[])
fprintf (stderr, "expecting no contexts, got %d\n", r);
exit (EXIT_FAILURE);
}
+ if (!p.freed) {
+ fprintf (stderr, "callback not freed by libnbd\n");
+ exit (EXIT_FAILURE);
+ }
/* Third pass: specific query should have one match. */
p = (struct progress) { .count = 0 };
@@ -129,7 +157,8 @@ main (int argc, char *argv[])
free (tmp);
r = nbd_opt_list_meta_context (nbd,
(nbd_context_callback) { .callback = check,
- .user_data = &p});
+ .user_data = &p,
+ .free = cb_free});
if (r == -1) {
fprintf (stderr, "%s\n", nbd_get_error ());
exit (EXIT_FAILURE);
@@ -138,6 +167,10 @@ main (int argc, char *argv[])
fprintf (stderr, "expecting exactly one context, got %d\n", r);
exit (EXIT_FAILURE);
}
+ if (!p.freed) {
+ fprintf (stderr, "callback not freed by libnbd\n");
+ exit (EXIT_FAILURE);
+ }
/* Fourth pass: nbd_opt_list_meta_context is stateless, so it should
* not wipe status learned during nbd_opt_info().
@@ -182,7 +215,8 @@ main (int argc, char *argv[])
p = (struct progress) { .count = 0 };
r = nbd_opt_list_meta_context (nbd,
(nbd_context_callback) { .callback = check,
- .user_data = &p});
+ .user_data = &p,
+ .free = cb_free});
if (r == -1) {
fprintf (stderr, "%s\n", nbd_get_error ());
exit (EXIT_FAILURE);
@@ -191,6 +225,10 @@ main (int argc, char *argv[])
fprintf (stderr, "expecting no contexts, got %d\n", r);
exit (EXIT_FAILURE);
}
+ if (!p.freed) {
+ fprintf (stderr, "callback not freed by libnbd\n");
+ exit (EXIT_FAILURE);
+ }
r = nbd_get_size (nbd);
if (r == -1) {
fprintf (stderr, "%s\n", nbd_get_error ());
@@ -219,7 +257,8 @@ main (int argc, char *argv[])
}
r = nbd_opt_list_meta_context (nbd,
(nbd_context_callback) { .callback = check,
- .user_data = &p});
+ .user_data = &p,
+ .free = cb_free});
if (r == -1) {
fprintf (stderr, "%s\n", nbd_get_error ());
exit (EXIT_FAILURE);
@@ -228,6 +267,10 @@ main (int argc, char *argv[])
fprintf (stderr, "expecting at least one context, got %d\n", r);
exit (EXIT_FAILURE);
}
+ if (!p.freed) {
+ fprintf (stderr, "callback not freed by libnbd\n");
+ exit (EXIT_FAILURE);
+ }
nbd_opt_abort (nbd);
nbd_close (nbd);
@@ -248,7 +291,8 @@ main (int argc, char *argv[])
p = (struct progress) { .count = 0 };
r = nbd_opt_list_meta_context (nbd,
(nbd_context_callback) { .callback = check,
- .user_data = &p});
+ .user_data = &p,
+ .free = cb_free});
if (r == -1) {
if (nbd_stats_bytes_sent (nbd) == bytes) {
fprintf (stderr, "bug: client failed to send request\n");
@@ -268,6 +312,10 @@ main (int argc, char *argv[])
exit (EXIT_FAILURE);
}
}
+ if (!p.freed) {
+ fprintf (stderr, "callback not freed by libnbd\n");
+ exit (EXIT_FAILURE);
+ }
/* Now enable structured replies, and a retry should pass. */
r = nbd_opt_structured_reply (nbd);
@@ -283,7 +331,8 @@ main (int argc, char *argv[])
p = (struct progress) { .count = 0 };
r = nbd_opt_list_meta_context (nbd,
(nbd_context_callback) { .callback = check,
- .user_data = &p});
+ .user_data = &p,
+ .free = cb_free});
if (r == -1) {
fprintf (stderr, "%s\n", nbd_get_error ());
exit (EXIT_FAILURE);
@@ -297,6 +346,10 @@ main (int argc, char *argv[])
fprintf (stderr, "server did not reply with base:allocation\n");
exit (EXIT_FAILURE);
}
+ if (!p.freed) {
+ fprintf (stderr, "callback not freed by libnbd\n");
+ exit (EXIT_FAILURE);
+ }
nbd_opt_abort (nbd);
nbd_close (nbd);
diff --git a/tests/opt-list.c b/tests/opt-list.c
index 692b292f..74a0c15e 100644
--- a/tests/opt-list.c
+++ b/tests/opt-list.c
@@ -34,6 +34,7 @@
struct progress {
int id;
int visit;
+ bool freed;
};
static int
@@ -41,6 +42,11 @@ check (void *user_data, const char *name, const char *description)
{
struct progress *p = user_data;
+ if (p->freed) {
+ fprintf (stderr, "use after free callback");
+ exit (EXIT_FAILURE);
+ }
+
if (*description) {
fprintf (stderr, "unexpected description for id %d visit %d: %s\n",
p->id, p->visit, description);
@@ -92,6 +98,18 @@ check (void *user_data, const char *name, const char *description)
return 0;
}
+static void
+cb_free (void *user_data)
+{
+ struct progress *p = user_data;
+
+ if (p->freed) {
+ fprintf (stderr, "too many free callbacks");
+ exit (EXIT_FAILURE);
+ }
+ p->freed = true;
+}
+
static struct nbd_handle*
prepare (int i)
{
@@ -133,7 +151,8 @@ main (int argc, char *argv[])
nbd = prepare (0);
p = (struct progress) { .id = 0 };
r = nbd_opt_list (nbd, (nbd_list_callback) { .callback = check,
- .user_data = &p});
+ .user_data = &p,
+ .free = cb_free});
if (r != -1) {
fprintf (stderr, "expected error after opt_list\n");
exit (EXIT_FAILURE);
@@ -142,13 +161,18 @@ main (int argc, char *argv[])
fprintf (stderr, "callback called unexpectedly\n");
exit (EXIT_FAILURE);
}
+ if (!p.freed) {
+ fprintf (stderr, "callback not freed by libnbd\n");
+ exit (EXIT_FAILURE);
+ }
cleanup (nbd);
/* Second pass: server advertises 'a' and 'b'. */
nbd = prepare (1);
p = (struct progress) { .id = 1 };
r = nbd_opt_list (nbd, (nbd_list_callback) { .callback = check,
- .user_data = &p});
+ .user_data = &p,
+ .free = cb_free});
if (r == -1) {
fprintf (stderr, "%s\n", nbd_get_error ());
exit (EXIT_FAILURE);
@@ -158,13 +182,18 @@ main (int argc, char *argv[])
r);
exit (EXIT_FAILURE);
}
+ if (!p.freed) {
+ fprintf (stderr, "callback not freed by libnbd\n");
+ exit (EXIT_FAILURE);
+ }
cleanup (nbd);
/* Third pass: server advertises empty list. */
nbd = prepare (2);
p = (struct progress) { .id = 2 };
r = nbd_opt_list (nbd, (nbd_list_callback) { .callback = check,
- .user_data = &p});
+ .user_data = &p,
+ .free = cb_free});
if (r == -1) {
fprintf (stderr, "%s\n", nbd_get_error ());
exit (EXIT_FAILURE);
@@ -174,13 +203,18 @@ main (int argc, char *argv[])
r);
exit (EXIT_FAILURE);
}
+ if (!p.freed) {
+ fprintf (stderr, "callback not freed by libnbd\n");
+ exit (EXIT_FAILURE);
+ }
cleanup (nbd);
/* Final pass: server advertises 'a'. */
nbd = prepare (3);
p = (struct progress) { .id = 3 };
r = nbd_opt_list (nbd, (nbd_list_callback) { .callback = check,
- .user_data = &p});
+ .user_data = &p,
+ .free = cb_free});
if (r == -1) {
fprintf (stderr, "%s\n", nbd_get_error ());
exit (EXIT_FAILURE);
@@ -190,6 +224,10 @@ main (int argc, char *argv[])
r);
exit (EXIT_FAILURE);
}
+ if (!p.freed) {
+ fprintf (stderr, "callback not freed by libnbd\n");
+ exit (EXIT_FAILURE);
+ }
cleanup (nbd);
exit (EXIT_SUCCESS);
--
2.41.0
1 year, 3 months
Rust Bindings for Libnbd
by Tage Johansson
This is Tage. I am a Google Summer of Code contributer this summer
working on Rust bindings for Libnbd. So far, I have made a very basic
first draft which is far from complete. But I'll send the patches anyway
so you can take a look.
I have created "generator/Rust.ml" which generates Rust code for
constants, flags, enums and handle calls. The top level "rust/"
directory contains the actual Rust crate. The make script generates
documentation with Rustdoc which can be found in
"rust/target/doc/libnbd/index.html".
To generate low level bindings,
[rust-bindgen](https://github.com/rust-lang/rust-bindgen) is used. This
means that Clang is required to build the bindings. I plan on removing
this dependency in the future though.
Feel free to send any comments and ideas, butI emphasize again that this
is just a first draft, and there is much more work to do.
Best regards,
Tage
1 year, 3 months
OCaml 5.0 warning
by Tage Johansson
When running make with OCaml 5.0 installed, I get some warnings
complaining about changed lib directory layout. It suggests adding
"-I +unix -I +str" to the invocation of ocamlc. So this patch does just
that.
It does silence the warnings for me, but someone should probably test
that it works with older versions of OCaml as well.
See the commit message for more details.
Best regards,
Tage
1 year, 3 months
[libguestfs PATCH] lib: remove guestfs_int_cmd_clear_close_files()
by Laszlo Ersek
The last (only?) caller of guestfs_int_cmd_clear_close_files() disappeared
in commit e4c396888056 ("lib/info: Remove /dev/fd hacking and pass a true
filename to qemu-img info.", 2018-01-23), part of v1.37.36.
Simplify the code by removing guestfs_int_cmd_clear_close_files().
Signed-off-by: Laszlo Ersek <lersek(a)redhat.com>
---
lib/guestfs-internal.h | 1 -
lib/command.c | 37 ++++++++++---------------------------
2 files changed, 10 insertions(+), 28 deletions(-)
diff --git a/lib/guestfs-internal.h b/lib/guestfs-internal.h
index fb55e02614f5..c7ef32277e93 100644
--- a/lib/guestfs-internal.h
+++ b/lib/guestfs-internal.h
@@ -751,7 +751,6 @@ extern void guestfs_int_cmd_set_stdout_callback (struct command *, cmd_stdout_ca
extern void guestfs_int_cmd_set_stderr_to_stdout (struct command *);
extern void guestfs_int_cmd_set_child_rlimit (struct command *, int resource, long limit);
extern void guestfs_int_cmd_clear_capture_errors (struct command *);
-extern void guestfs_int_cmd_clear_close_files (struct command *);
extern void guestfs_int_cmd_set_child_callback (struct command *, cmd_child_callback child_callback, void *data);
extern int guestfs_int_cmd_run (struct command *);
extern void guestfs_int_cmd_close (struct command *);
diff --git a/lib/command.c b/lib/command.c
index 515ef624bd96..82a47bafa9e5 100644
--- a/lib/command.c
+++ b/lib/command.c
@@ -152,9 +152,6 @@ struct command
/* When using the pipe_* APIs, stderr is pointed to a temporary file. */
char *error_file;
- /* Close file descriptors (defaults to true). */
- bool close_files;
-
/* Supply a callback to receive stdout. */
cmd_stdout_callback stdout_callback;
void *stdout_data;
@@ -186,7 +183,6 @@ guestfs_int_new_command (guestfs_h *g)
cmd = safe_calloc (g, 1, sizeof *cmd);
cmd->g = g;
cmd->capture_errors = true;
- cmd->close_files = true;
cmd->errorfd = -1;
cmd->outfd = -1;
return cmd;
@@ -358,17 +354,6 @@ guestfs_int_cmd_clear_capture_errors (struct command *cmd)
cmd->capture_errors = false;
}
-/**
- * Don't close file descriptors after the fork.
- *
- * XXX Should allow single fds to be sent to child process.
- */
-void
-guestfs_int_cmd_clear_close_files (struct command *cmd)
-{
- cmd->close_files = false;
-}
-
/**
* Set a function to be executed in the child, right before the
* execution. Can be used to setup the child, for example changing
@@ -564,18 +549,16 @@ run_child (struct command *cmd, char **env)
for (i = 1; i < NSIG; ++i)
sigaction (i, &sa, NULL);
- if (cmd->close_files) {
- /* Close all other file descriptors. This ensures that we don't
- * hold open (eg) pipes from the parent process.
- */
- max_fd = sysconf (_SC_OPEN_MAX);
- if (max_fd == -1)
- max_fd = 1024;
- if (max_fd > 65536)
- max_fd = 65536; /* bound the amount of work we do here */
- for (fd = 3; fd < max_fd; ++fd)
- close (fd);
- }
+ /* Close all other file descriptors. This ensures that we don't
+ * hold open (eg) pipes from the parent process.
+ */
+ max_fd = sysconf (_SC_OPEN_MAX);
+ if (max_fd == -1)
+ max_fd = 1024;
+ if (max_fd > 65536)
+ max_fd = 65536; /* bound the amount of work we do here */
+ for (fd = 3; fd < max_fd; ++fd)
+ close (fd);
/* Set the umask for all subcommands to something sensible (RHBZ#610880). */
umask (022);
1 year, 3 months
[libnbd PATCH v4 0/4] Saner reply header layout
by Eric Blake
This was v3 patch 2/22, reworked to address the confusion about how a
structured reply header is read in two pieces before getting to the
payload portion.
I'm still working on rebasing the rest of my v3 series (patches 1,
3-22) from other comments given, but this seemed independent enough
that it's worth posting now rather than holding it up for the rest of
the series.
Eric Blake (4):
states: Document our reliance on type overlaps
generator: Finish parsing structured header in states-reply.c
generator: Rename states-reply-structured to states-reply-chunk
internal: Refactor layout of replies in sbuf
lib/internal.h | 16 ++-
generator/state_machine.ml | 53 ++++---
generator/states-reply.c | 65 +++++++--
...eply-structured.c => states-reply-chunk.c} | 129 ++++++++----------
generator/states-reply-simple.c | 4 +-
generator/Makefile.am | 2 +-
6 files changed, 143 insertions(+), 126 deletions(-)
rename generator/{states-reply-structured.c => states-reply-chunk.c} (78%)
--
2.40.1
1 year, 3 months
Libnbd asynchronous API with epoll
by Tage Johansson
Hello,
As part of the Rust bindings for Libnbd, I try to integrate the
asynchronous (aio_*) functions with Tokio
<https://docs.rs/tokio/latest/tokio/>, the most used asynchronous
runtime in Rust. However, in its eventloop, Tokio uses epoll(7) instead
of poll(2) (which is used internally in Libnbd). The difference is that
poll(2) uses level-triggered notifications as aposed to epoll(7) which
uses edge-triggered notifications. In short, the difference is that if
one would wait for a file discriptor to be readable, epoll would block
until the file descriptor changes from not readable to readable. So if
the file descriptor is already readable then epoll would block until it
becomes unreadable and readable again.
I am not sure how to integrate Libnbd into an edge-triggered system. The
following questions arises:
- After calling aio_get_direction(3), can I know that reading/writing
would actually block?
- After calling for example aio_notify_read(3), can I know that the next
reading from the file descriptor would block?
If the answers to both of these questions are no, the only solution I
can think of is to call poll(2) with a timeout of 0 as a non blocking
check if the file descriptor is ready. Only if the call to poll(2)
fails, I would use the epoll-driven eventloop to wait for changes.
What do you think about this? Do you have an easier solution to
integrate Libnbd with an edge-triggered eventloop?
Best regards,
Tage
1 year, 3 months
