[Libguestfs] [libnbd PATCH 0/4] Work towards fixing nbdcopy silent corruption

[Richard W.M. Jones]

On Wed, Feb 02, 2022 at 04:34:09PM +0100, Laszlo Ersek wrote:
> OK -- so end-to-end (IIUC), all I could learn, with reading !err in the
> OCaml callback, is whether an earlier invocation of the callback raised
> an exception (possibly indirectly, i.e. by returning -1 to the wrapper,
> or directly by raising an exception, or even by allowing a deeper
> exception to escape).

If the callback returned -1 it would cause an assert fail.  The only
way to return an error indication is to raise an exception (which
causes the wrapper to return -1 back to libnbd which then sets
cmd->error to !err || EPROTO).

BTW I noticed that we could be more clever about !err, but the
implementation is not as trivial as I thought so I only documented it:

https://gitlab.com/nbdkit/libnbd/-/commit/0e2dd4bf5b024889ed9e63dd2cef9a2def873332

> Eric said the extents array and the metacontext ID
> are still valid in this case, so eliminating the loop body would only be
> a speed-up, and the outermost call will throw an exception anyway.
> 
> ... What happens "if the server replies with NBD_REPLY_TYPE_ERROR first
> and then NBD_REPLY_TYPE_BLOCK_STATUS second (a custom server)"?

We jump through states %REPLY.STRUCTURED_REPLY.CHECK ->
%REPLY.STRUCTURED_REPLY.RECV_ERROR_MESSAGE ->
%REPLY.STRUCTURED_REPLY.RECV_ERROR_TAIL.

This will set cmd->error (although not if it was already set by a
callback).  Then it'll jump to %REPLY.FINISHED which calls the
completion callback.

Then it'll read NBD_REPLY_TYPE_BLOCK_STATUS, get confused and
desynchronize (the magic field will be different), resulting in the
handle becoming %DEAD.

I believe this is all correct behaviour, as far as that's possible
with a server that's doing weird stuff.

> Hmm I don't think it matters much: we already assume that the entries
> make sense (e.g. we already assert() that the server does not send
> zero-length entries), so we'd just complete the inner loop (ignoring
> !err being nonzero), and then the NBD.block_status function would raise
> an exception. OK!

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