On Thu, Jul 13, 2023 at 09:53:58AM +0000, Tage Johansson wrote:> Apologize if resending, but I'm not sure my previous email was > actually delivered. > > > On 7/12/2023 10:33 PM, Eric Blake wrote: > > > >On Wed, Jul 12, 2023 at 03:19:59PM +0000, Tage Johansson wrote: > >>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. > >Can you paste the valgrind output showing the leak? > > > The Valgrind output is very Rust specific and only shows the Rust > allocations which goes into the completion callback and are lost. > > > But if you apply the following patch (which modifies > tests/opt-info.c) you shall see that the completion callback is not > called. > > I have replaced a call to `nbd_opt_info()` with a call to > `nbd_aio_opt_info()` and passed in a completion callback which just > calls `exit(EXIT_FAILURE)`. So if the completion callback is called > the test should fail, which it doesn't, at least not on my machine.Isn't that OK? Only .free is required to be called. Rich.> > diff --git a/tests/opt-info.c b/tests/opt-info.c > index 86a0608..ba5c72b 100644 > --- a/tests/opt-info.c > +++ b/tests/opt-info.c > @@ -30,6 +30,12 @@ > > ?#include <libnbd.h> > > +int > +my_completion_cb(void *_user_data, int *_err) > +{ > +? exit(EXIT_FAILURE); > +} > + > ?int > ?main (int argc, char *argv[]) > ?{ > @@ -201,7 +207,10 @@ main (int argc, char *argv[]) > ???? fprintf (stderr, "expecting export name 'good', got '%s'\n", s); > ???? exit (EXIT_FAILURE); > ?? } > -? if (nbd_opt_info (nbd) != -1) { > +? if (nbd_aio_opt_info ( > +??????? nbd, > +??????? (nbd_completion_callback) { .callback = my_completion_cb } > +????? ) != -1) { > ???? fprintf (stderr, "expecting error for opt_info\n"); > ???? exit (EXIT_FAILURE); > ?? } > > Best regards, > > Tage > >-- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com Fedora Windows cross-compiler. Compile Windows programs, test, and build Windows installers. Over 100 libraries supported. http://fedoraproject.org/wiki/MinGW
On 7/13/2023 1:03 PM, Richard W.M. Jones wrote:> On Thu, Jul 13, 2023 at 09:53:58AM +0000, Tage Johansson wrote: >> Apologize if resending, but I'm not sure my previous email was >> actually delivered. >> >> >> On 7/12/2023 10:33 PM, Eric Blake wrote: >> >> >>> On Wed, Jul 12, 2023 at 03:19:59PM +0000, Tage Johansson wrote: >>>> 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. >>> Can you paste the valgrind output showing the leak? >> >> The Valgrind output is very Rust specific and only shows the Rust >> allocations which goes into the completion callback and are lost. >> >> >> But if you apply the following patch (which modifies >> tests/opt-info.c) you shall see that the completion callback is not >> called. >> >> I have replaced a call to `nbd_opt_info()` with a call to >> `nbd_aio_opt_info()` and passed in a completion callback which just >> calls `exit(EXIT_FAILURE)`. So if the completion callback is called >> the test should fail, which it doesn't, at least not on my machine. > Isn't that OK? Only .free is required to be called.Of course, you are right. I will continue investigating why my Rust closure isn't freed though. Best regards, Tage
On Thu, Jul 13, 2023 at 12:03:24PM +0100, Richard W.M. Jones wrote:> On Thu, Jul 13, 2023 at 09:53:58AM +0000, Tage Johansson wrote: > > Apologize if resending, but I'm not sure my previous email was > > actually delivered. > > > > > > On 7/12/2023 10:33 PM, Eric Blake wrote: > > > > > > >On Wed, Jul 12, 2023 at 03:19:59PM +0000, Tage Johansson wrote: > > >>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. > > >Can you paste the valgrind output showing the leak? > > > > > > The Valgrind output is very Rust specific and only shows the Rust > > allocations which goes into the completion callback and are lost. > > > > > > But if you apply the following patch (which modifies > > tests/opt-info.c) you shall see that the completion callback is not > > called.Ah. I see. We are testing the synchronous command, but not the aio counterpart command. That is indeed a hole in the testsuite (even after my patch yesterday).> > > > I have replaced a call to `nbd_opt_info()` with a call to > > `nbd_aio_opt_info()` and passed in a completion callback which just > > calls `exit(EXIT_FAILURE)`. So if the completion callback is called > > the test should fail, which it doesn't, at least not on my machine. > > Isn't that OK? Only .free is required to be called.For the context callback (for opt_set_meta), .callback may be called zero, one, or multiple times, but .free should be called exactly once. But for the completion callback, I agree that the docs state that both .callback and .free should each be called exactly once ("On the other hand, if a completion callback is supplied (only possible with asynchronous commands), it will always be reached exactly once, and the completion callback must not ignore the value pointed to by C<error>."); we are indeed missing the call to .callback. I'll work on a patch. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org
On 7/13/2023 2:13 PM, Eric Blake wrote:> On Thu, Jul 13, 2023 at 12:03:24PM +0100, Richard W.M. Jones wrote: >> On Thu, Jul 13, 2023 at 09:53:58AM +0000, Tage Johansson wrote: >>> Apologize if resending, but I'm not sure my previous email was >>> actually delivered. >>> >>> >>> On 7/12/2023 10:33 PM, Eric Blake wrote: >>> >>> >>>> On Wed, Jul 12, 2023 at 03:19:59PM +0000, Tage Johansson wrote: >>>>> 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. >>>> Can you paste the valgrind output showing the leak? >>> >>> The Valgrind output is very Rust specific and only shows the Rust >>> allocations which goes into the completion callback and are lost. >>> >>> >>> But if you apply the following patch (which modifies >>> tests/opt-info.c) you shall see that the completion callback is not >>> called. > Ah. I see. We are testing the synchronous command, but not the aio > counterpart command. That is indeed a hole in the testsuite (even > after my patch yesterday). > >>> I have replaced a call to `nbd_opt_info()` with a call to >>> `nbd_aio_opt_info()` and passed in a completion callback which just >>> calls `exit(EXIT_FAILURE)`. So if the completion callback is called >>> the test should fail, which it doesn't, at least not on my machine. >> Isn't that OK? Only .free is required to be called. > For the context callback (for opt_set_meta), .callback may be called > zero, one, or multiple times, but .free should be called exactly once. > But for the completion callback, I agree that the docs state that both > .callback and .free should each be called exactly once ("On the other > hand, if a completion callback is supplied (only possible with > asynchronous commands), it will always be reached exactly once, and > the completion callback must not ignore the value pointed to by > C<error>."); we are indeed missing the call to .callback. I'll work > on a patch.Ok, so if I understand correctly the completion callback should actually be called exactly once, right? Then I will not have to adjust my code. I am storing the completion callback as an `FnOnce` <https://doc.rust-lang.org/std/ops/trait.FnOnce.html>, so it should free itself when it is called. If it will be called zero or one time I could store it in an `Option` <https://doc.rust-lang.org/std/option/enum.Option.html> and take <https://doc.rust-lang.org/std/option/enum.Option.html#method.take> it when it is called. The free callback would then free that Option. But if the completion callback will be called exactly once that'll be unnecessary. Best regards, Tage -------------- next part -------------- An HTML attachment was scrubbed... URL: <http://listman.redhat.com/archives/libguestfs/attachments/20230713/8cb98dc4/attachment-0001.htm>
On Thu, Jul 13, 2023 at 07:13:37AM -0500, Eric Blake wrote:> > > I have replaced a call to `nbd_opt_info()` with a call to > > > `nbd_aio_opt_info()` and passed in a completion callback which just > > > calls `exit(EXIT_FAILURE)`. So if the completion callback is called > > > the test should fail, which it doesn't, at least not on my machine. > > > > Isn't that OK? Only .free is required to be called. > > For the context callback (for opt_set_meta), .callback may be called > zero, one, or multiple times, but .free should be called exactly once. > But for the completion callback, I agree that the docs state that both > .callback and .free should each be called exactly once ("On the other > hand, if a completion callback is supplied (only possible with > asynchronous commands), it will always be reached exactly once, and > the completion callback must not ignore the value pointed to by > C<error>."); we are indeed missing the call to .callback. I'll work > on a patch.Eww, the bug is bigger than just nbd_aio_opt* not always calling completion.callback exactly once. With just this diff (to be folded into the larger patch I'm working on), I'm getting an assertion failure that we fail to call completion.callback for nbd_aio_pread_structured when calling nbd_close() prior to the command running to completion, so I'll have to fix that too. diff --git i/tests/closure-lifetimes.c w/tests/closure-lifetimes.c index 9bb4e120..c5ad4c10 100644 --- i/tests/closure-lifetimes.c +++ w/tests/closure-lifetimes.c @@ -66,6 +66,7 @@ read_cb (void *opaque, uint64_t offset, unsigned status, int *error) { assert (!read_cb_freed); + assert (!completion_cb_called); read_cb_called++; return 0; } @@ -73,6 +74,7 @@ read_cb (void *opaque, static void read_cb_free (void *opaque) { + assert (completion_cb_called); read_cb_freed++; } @@ -81,6 +83,7 @@ block_status_cb (void *opaque, const char *meta, uint64_t offset, uint32_t *entries, size_t nr_entries, int *error) { assert (!block_status_cb_freed); + assert (!completion_cb_called); block_status_cb_called++; return 0; } @@ -88,6 +91,7 @@ block_status_cb (void *opaque, const char *meta, uint64_t offset, static void block_status_cb_free (void *opaque) { + assert (completion_cb_called); block_status_cb_freed++; } @@ -102,6 +106,7 @@ completion_cb (void *opaque, int *error) static void completion_cb_free (void *opaque) { + assert (completion_cb_called); completion_cb_freed++; } -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org