[Libguestfs] [nbdkit PATCH 3/4] server: Close backends if a filter's .open fails
We already had code that ensured that a filter's .open cannot succeed
unless its backend is also open. However, we did not have the
converse: if a filter's .open fails, the backend handle could still be
open. As long as we don't use a connection after a filter's open
failure, the plugin stays open, the backend eventually gets closed
when the connection goes away. But now that we allow retries after
NBD_OPT_GO failure, a second attempt from the same connection could
run into a backend that is still open - fix it by closing the backend
right away, rather than delaying until connection close. (This one is
not easy to trigger without a custom client).
Conversely, now that we have the retry filter, we can end up in the
situation where a filter is open, but the backend is not, when reopen
fails. And that scenario is now more common when a filter .open
fails, with the above change to leave the plugin closed. Since our
connection code only called the close chain if the plugin was still
open, this can result in memory leaks, as shown by:
$ NBDKIT_VALGRIND=1 ./nbdkit -U- --filter=log --filter=retry \
null size=512 retries=1 retry-delay=1 \
--run 'qemu-io -f raw $nbd -c "r 0 1"' logfile=/dev/stderr
Fix it by always running the close chain regardless of whether the
plugin is open.
Signed-off-by: Eric Blake <eblake(a)redhat.com>
---
server/backend.c | 5 ++++-
server/connections.c | 2 +-
2 files changed, 5 insertions(+), 2 deletions(-)
diff --git a/server/backend.c b/server/backend.c
index 64267bfe..5cbbe2c1 100644
--- a/server/backend.c
+++ b/server/backend.c
@@ -186,8 +186,11 @@ backend_open (struct backend *b, struct connection *conn, int
readonly)
if (b->i) /* A filter must not succeed unless its backend did also */
assert (conn->handles[b->i - 1].handle);
}
- else
+ else {
assert (h->handle == NULL);
+ if (b->i) /* Do not strand backend if this layer failed */
+ backend_close (b->next, conn);
+ }
return r;
}
diff --git a/server/connections.c b/server/connections.c
index 27cf202b..df5e09af 100644
--- a/server/connections.c
+++ b/server/connections.c
@@ -360,7 +360,7 @@ free_connection (struct connection *conn)
* thread will be in the process of unloading it. The plugin.unload
* callback should always be called.
*/
- if (!quit && connection_get_handle (conn, 0)) {
+ if (!quit) {
lock_request (conn);
backend_close (backend, conn);
unlock_request (conn);
--
2.21.0