Re: [Libguestfs] Few libnbd questions/concerns

On 6/25/19 9:25 AM, Eric Blake wrote: >>> When running a program with `nbdkit -U - --run ...`, the $nbd parameter gets >>> expanded to nbd:unix:/path/to/socket. When this string is passed to >>> nbd_connect_uri(), it does not return an error (even though it is not a valid >>> URL), but what's more it treats it as "nbd://localhost", which might be a completely different server (that actually happened to me and it was kind of confusing). >> >> There's a bit of magic in nbdkit about $nbd. It also predates our >> standardization effort for NBD URLs which is going on upstream. >> >> That needs to be fixed, but in the meantime just use $unixsocket >> instead (see nbdkit-captive(1) man page for the full details). > > In the meantime, I don't mind working on the quick fix for rejecting an > invalid URI. Done. > >> >>> When nbd_block_status() callback returns -1, the nbd_block_status() function >>> reports: >>> >>> command failed: Protocol error (errno = 71) >>> >>> which is a bit confusing to me. I could be nicer to have that report that it >>> really was the callback that caused this. >> >> Not sure about this one - Eric? > > Right now, a failed callback will cause the overall nbd_block_status() > to fail with whatever errno the callback left behind, with EPROTO as the > default. I'm guessing you didn't set an errno when you returned -1? Is > there a better default we should use than EPROTO? Is there really a way > to diagnose without an errno value that the callback failed but didn't > set errno? > My pending patch to take advantage of Mutable(Int) in the callback, makes this a bit nicer, as you can then control the error status by assigning into the parameter rather than by what got left in errno. > There's also the related question - my patches to add > nbd_pread_structured are set up to call the callback for as many chunks > as the server sends, even if an earlier invocation of the callback > fails, but to still preserve the errno of the first failed callback > (again defaulting to EPROTO). Should nbd_block_status also call the > callback for all chunks from the server, rather than stopping the use of > further callbacks once the first callback fails, if only for consistency > between the two? So now I'm leaning towards making that change, particularly since I'm also working on adding a nbd_aio_FOO_notify() for each command that takes a callback that also takes an error pointer. > >>> One last thing is that I could not find any definition for the flags >>> of "base:allocation" metadata (NBD_STATE_HOLE and NBD_STATE_ZERO). >>> It might just be things that are still missing due to an early stage >>> of libnbd, or it might not belong there or it might be me misusing >>> something. >> >> Have a look at the NBD protocol doc: >> >> https://github.com/NetworkBlockDevice/nbd/blob/master/doc/proto.md > > We could add convenience #defines into libnbd.h if needed, which would > work for the base:allocation context, but it would not help any other > context (such as qemu:dirty-bitmap:FOO) which has other definitions for > the returned bits. But the fact that you have to call > nbd_add_meta_context(h, "base:allocation") is also awkward; if we add > convenience macros for the results, we probably also want to support > nbd_add_meta_context(h, LIBNBD_CONTEXT_BASE_ALLOCATION). Still needs to be done.

Another thing I just noticed: nbd_poll returns 0 both for timeout and for when it notified the state machine about POLLIN or POLLOUT making progress. We probably want to switch it to return 0 on timeout and 1 when it made progress, so that a user can detect timeout (all our internal callers pass -1 for the timeout parameter, and thus can't time out, but external callers may care).

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