Libguestfs - Feb 2023 - no way to force-close NBD handle in nbdsh

I'm out of time this weekend, but while trying to write a test for an
nbdkit bug fix (a nasty assertion failure when the client disconnects
uncleanly without NBD_CMD_DISC, which is what nbdcopy does if it gets
an EIO read response from the server), I realized it is extremely hard
to trigger this using nbdsh, but easy to do in C or the Go bindings.

In the Go bindings, we intentionally coded things so that the Go
structure knows if its underlying C pointer is live or not; it exposes
a way to force early closure, then the bindings return a
closed_handle_error() if early closure has happened.  We probably need
to support something similar in the Python bindings.

As a short-term hack, I tried directly calling h.__del__() - this is
not a good idea, as our current __del__ implementation is not designed
to be called twice, and when the later garbage collection calls it
again, we segfault trying to free invalid memory.  But less drastic
measures, such as:

import gc
del h
h = None
gc.collect()

were still insufficient to trigger a normal __del__ of h, because I
can't figure out how to force the garbage collector to see that I want
to close the handle but keep python open.

In the end, I did get a working test written (by just quit()ing python
instead of trying to keep it alive).  See my next email for the nbdkit
patch that spawned my request here.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

On Fri, Feb 24, 2023 at 04:41:20PM -0600, Eric Blake
wrote:
> I'm out of time this weekend, but while trying to write a test for an
> nbdkit bug fix (a nasty assertion failure when the client disconnects
> uncleanly without NBD_CMD_DISC, which is what nbdcopy does if it gets
> an EIO read response from the server), I realized it is extremely hard
> to trigger this using nbdsh, but easy to do in C or the Go bindings.
> 
> In the Go bindings, we intentionally coded things so that the Go
> structure knows if its underlying C pointer is live or not; it exposes
> a way to force early closure, then the bindings return a
> closed_handle_error() if early closure has happened.  We probably need
> to support something similar in the Python bindings.
> 
> As a short-term hack, I tried directly calling h.__del__() - this is
> not a good idea, as our current __del__ implementation is not designed
> to be called twice, and when the later garbage collection calls it
> again, we segfault trying to free invalid memory.
This is a bug :-(
> But less drastic
> measures, such as:
> 
> import gc
> del h
> h = None
> gc.collect()
> 
> were still insufficient to trigger a normal __del__ of h, because I
> can't figure out how to force the garbage collector to see that I want
> to close the handle but keep python open.
> 
> In the end, I did get a working test written (by just quit()ing python
> instead of trying to keep it alive).  See my next email for the nbdkit
> patch that spawned my request here.
I think we want an explicit h.close method, just like we already have
in OCaml and golang bindings.  This also implies a "handle is closed"
error/exception which is raised when you call another method on the
handle after it is closed.

I thought that this was how the Python bindings already worked, but
I've checked and it's as you say - there's no method to explicitly
close the handle.

Such a change is backwards compatible with existing clients (since you
can still let the handle go out of scope and have it closed
automatically).

I can have a go at adding this since I unexpectedly have some free
time this weekend.

Rich.

-- 
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