[Libguestfs] [libnbd PATCH v2 6/6] info: Tolerate missing size

[Laszlo Ersek]

On 9/21/23 22:58, Eric Blake wrote:
> As previous patches showed, the NBD spec does not yet forbid a server
> sending us a size that does not fit in int64_t.  We should gracefully
> handle this during nbdinfo, rather than giving up early.
> 
> With the same one-line hack to qemu to set the most significant bit of
> the export size, output changes from pre-patch:
> 
> $ ./run nbdinfo 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
> 
> to post-patch:
> 
> $ ./run nbdinfo nbd://localhost
> protocol: newstyle-fixed without TLS, using extended packets
> ...
> 	block_size_maximum: 33554432
> 
> or
> 
> $ ./run nbdinfo nbd://localhost --json
> {
> "protocol": "newstyle-fixed",
> ...
> 	"block_size_maximum": 33554432,
> 	"export-size-str": "unavailable"
> 	} ]
> }
> 
> Sadly, since writing a server with such large export sizes requires a
> one-off hack, I don't see the point in adding a unit test.
> 
> Signed-off-by: Eric Blake <eblake at redhat.com>
> ---
>  info/show.c | 25 +++++++++++++------------
>  1 file changed, 13 insertions(+), 12 deletions(-)
> 
> diff --git a/info/show.c b/info/show.c
> index a71d837e..3d80545e 100644
> --- a/info/show.c
> +++ b/info/show.c
> @@ -46,7 +46,7 @@ show_one_export (struct nbd_handle *nbd, const char *desc,
>                   bool first, bool last)
>  {
>    int64_t i, size;
> -  char size_str[HUMAN_SIZE_LONGEST];
> +  char size_str[HUMAN_SIZE_LONGEST] = "unavailable";
>    bool human_size_flag;
>    char *export_name = NULL;
>    char *export_desc = NULL;
> @@ -89,13 +89,10 @@ show_one_export (struct nbd_handle *nbd, const char *desc,
>      return false;
>    }
>    size = nbd_get_size (nbd);
> -  if (size == -1) {
> -    fprintf (stderr, "%s: %s\n", progname, nbd_get_error ());
> -    exit (EXIT_FAILURE);
> +  if (size >= 0) {
> +    human_size (size_str, size, &human_size_flag);
>    }
> 
> -  human_size (size_str, size, &human_size_flag);
> -
>    if (uri_is_meaningful ())
>      uri = nbd_get_uri (nbd);
> 
> @@ -130,7 +127,8 @@ show_one_export (struct nbd_handle *nbd, const char *desc,
>      show_context = true;
> 
>    /* Get content last, as it moves the connection out of negotiating */
> -  content = get_content (nbd, size);
> +  if (size >= 0)
> +    content = get_content (nbd, size);
> 
>    if (!json_output) {
>      ansi_colour (ANSI_FG_BOLD_BLACK, fp);
> @@ -140,10 +138,12 @@ show_one_export (struct nbd_handle *nbd, const char *desc,
>      fprintf (fp, ":\n");
>      if (desc && *desc)
>        fprintf (fp, "\tdescription: %s\n", desc);
> -    if (human_size_flag)
> -      fprintf (fp, "\texport-size: %" PRIi64 " (%s)\n", size, size_str);
> -    else
> -      fprintf (fp, "\texport-size: %" PRIi64 "\n", size);
> +    if (size >= 0) {
> +      if (human_size_flag)
> +        fprintf (fp, "\texport-size: %" PRIi64 " (%s)\n", size, size_str);
> +      else
> +        fprintf (fp, "\texport-size: %" PRIi64 "\n", size);
> +    }
>      if (content)
>        fprintf (fp, "\tcontent: %s\n", content);
>      if (uri)
> @@ -273,7 +273,8 @@ show_one_export (struct nbd_handle *nbd, const char *desc,
>                 block_maximum);
> 
>      /* Put this one at the end because of the stupid comma thing in JSON. */
> -    fprintf (fp, "\t\"export-size\": %" PRIi64 ",\n", size);
> +    if (size >= 0)
> +      fprintf (fp, "\t\"export-size\": %" PRIi64 ",\n", size);
>      fprintf (fp, "\t\"export-size-str\": \"%s\"\n", size_str);
> 
>      if (last)

Assuming the size is unavailable, the non-JSON output includes neither
the numeric nor the human-readable size, whereas the JSON output still
includes the human-readable size (as "unavailable").

Is this difference intentional? I'd have thought that the JSON output
should similarly exclude the human-readable size string in case the size
were not available -- consequently, there'd be no need for initializing
"size_str" to "unavailable", because "size_str" would never be printed
if the size were unavailable.

If OTOH the behavior is intentional, then

Reviewed-by: Laszlo Ersek <lersek at redhat.com>