On Fri, Sep 22, 2023 at 12:33:36PM -0500, Eric Blake wrote:
> There are a number of ways in which gathering information can fail.
> But when possible, it makes sense to let the server know that we are
> done talking, as it minimizes the likelihood that nbdinfo's error
> message will be obscured by an accompanying error message by the
> server complaining about an unclean disconnect.
>
> For example, with a one-off qemu temporarily patched as mentioned in
> commit 0f8ee8c6 to advertise sizes larger than 2^63, kicking off
> 'qemu-nbd -f raw -r file &' produces:
>
> pre-patch:
>
> $ ./run nbdinfo --size nbd://localhost
> /home/eblake/libnbd/info/.libs/nbdinfo: nbd_get_size: server claims size
> 9223372036854781440 which does not fit in signed result: Value too large for
> defined data type
> qemu-nbd: option negotiation failed: Failed to read opts magic: Unexpected
> end-of-file before all data were read
This doesn't necessarily seem like a bug?
This feels like quite a significant change in behaviour to me. In
particular I'm worried if it interacts in some subtle way with the
forking done by the "[ ... ]" syntax for running servers on the
command line (or any of the other ways that libnbd might fork/exec).
Can we hold this patch until after 1.18 is released and then put it
in? Should only be a week or two.
Provisionally ACKed for post-1.18
Rich.
> post-patch:
> $ ./run nbdinfo --size nbd://localhost
> /home/eblake/libnbd/info/.libs/nbdinfo: nbd_get_size: server claims size
> 9223372036854781440 which does not fit in signed result: Value too large for
> defined data type
>
> if nbdinfo is run in the same terminal as qemu-nbd.
>
> Signed-off-by: Eric Blake <ebl...@redhat.com>
> ---
> info/main.c | 13 +++++++++++++
> 1 file changed, 13 insertions(+)
>
> diff --git a/info/main.c b/info/main.c
> index c8248c61..05f19f43 100644
> --- a/info/main.c
> +++ b/info/main.c
> @@ -88,6 +88,17 @@ usage (FILE *fp, int exitcode)
> exit (exitcode);
> }
>
> +void
> +clean_shutdown (void)
> +{
> + /* If we are connected but detect an error, try to give the server
> + * notice that we are done talking. Ignore failures, as this is
> + * only a courtesy measure.
> + */
> + if (nbd)
> + nbd_shutdown (nbd, 0);
> +}
> +
> int
> main (int argc, char *argv[])
> {
> @@ -277,6 +288,7 @@ main (int argc, char *argv[])
> fprintf (stderr, "%s: %s\n", progname, nbd_get_error ());
> exit (EXIT_FAILURE);
> }
> + atexit (clean_shutdown);
> nbd_set_uri_allow_local_file (nbd, true); /* Allow ?tls-psk-file. */
>
> /* Set optional modes in the handle. */
> @@ -353,6 +365,7 @@ main (int argc, char *argv[])
> free_exports ();
> nbd_shutdown (nbd, 0);
> nbd_close (nbd);
> + nbd = NULL;
>
> /* Close the output stream and copy it to the real stdout. */
> if (fclose (fp) == EOF) {
> --
> 2.41.0
>
--
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-top is 'top' for virtual machines. Tiny program with many
powerful monitoring features, net stats, disk stats, logging, etc.
http://people.redhat.com/~rjones/virt-top
_______________________________________________
Libguestfs mailing list
Libguestfs@redhat.com
https://listman.redhat.com/mailman/listinfo/libguestfs
