Re: [Libguestfs] libnbd: When are callbacks freed

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.