[Libguestfs] Re: [PATCH nbdkit v2 5/5] server: Send the last error to the NBD client

On Wed, Jul 24, 2024 at 11:45:45AM GMT, Richard W.M. Jones wrote: > This sends the last error saved in the connection handle back to the > NBD client. This is informational and best effort. > > qemu reports the error already, for example: > > $ nbdkit --log=null \ > eval open=' echo EPERM Go Away >&2; exit 1 ' get_size=' echo 100 ' \ > --run 'qemu-img info "$uri"' > qemu-img: Could not open 'nbd+unix://?socket=/tmp/nbdkitIDl6iy/socket': Requested export not available > server reported: /tmp/nbdkitRDAfXH/open: Go Away > > This goes back to at least qemu 2.12.0 (RHEL 7) and possibly earlier, > so we can just assume that qemu does this for the test. > > libnbd requires a patch to display this information. > --- ... > +++ b/server/protocol-handshake-newstyle.c > fixed_new_option_reply.magic = htobe64 (NBD_REP_MAGIC); > fixed_new_option_reply.option = htobe32 (option); > fixed_new_option_reply.reply = htobe32 (reply); > - fixed_new_option_reply.replylen = htobe32 (0); > + fixed_new_option_reply.replylen = htobe32 (replylen); > > debug ("replying to %s with %s", name_of_nbd_opt (option), > name_of_nbd_rep (reply)); > if (conn->send (&fixed_new_option_reply, > - sizeof fixed_new_option_reply, 0) == -1) { > + sizeof fixed_new_option_reply, > + replylen > 0 ? SEND_MORE : 0) == -1) { > + err: > /* The protocol document says that the client is allowed to simply > * drop the connection after sending NBD_OPT_ABORT, or may read > * the reply. > @@ -77,6 +93,8 @@ send_newstyle_option_reply (uint32_t option, uint32_t reply) > nbdkit_error ("write: %s: %m", name_of_nbd_opt (option)); > return -1; > } > + if (replylen > 0 && conn->send (last_error, replylen, 0) == -1) > + goto err; Jumping backwards to the err label is awkward; in the past, Laszlo has asked that we try harder to consolidate error handling at the end so we don't have to look backwards. Otherwise, this looks like a nice series. Thread-locals can be a bit hard to reason about, but I'm not seeing any obvious bugs. > +# Test informational error messages sent to the NBD client. > +# qemu-img supports this since at least 2.12.0. > + > +requires_run > +requires_plugin eval > +requires qemu-img --version > + > +out=last-error.out > +rm -f $out > +cleanup_fn rm -f $out > + > +export out > + > +nbdkit eval \ > + open=' echo EPERM Go Away >&2; exit 1 ' get_size=' echo 0 ' \ > + --run ' qemu-img info "$uri" > $out 2>&1 ||: ' > +cat $out > + > +grep "Go Away" $out Cool test! Overall, I'm liking the series.