On Fri, Sep 22, 2023 at 02:19:32PM +0200, Laszlo Ersek wrote:
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
>
[1]
>
> 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(a)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;
Uninitialized...
> 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);
...and relies on human_size() to initialize it, but that is now
conditionally skipped. Would matter if...
> -
> 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);
> or
>
> $ ./run nbdinfo nbd://localhost --json
> {
> "protocol": "newstyle-fixed",
> ...
> "block_size_maximum": 33554432,
> "export-size-str": "unavailable"
> } ]
> }
> @@ -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);
...branching on the variable is guarded by anything different than the
condition that controlled whether the variable was set in the first
place. Although it is not broken here, it is a trap for the unwary,
so I'll be initializing human_size_flag to false at its declaration.
> + else
> + fprintf (fp, "\texport-size: %" PRIi64 "\n", size);
> + }
[2]
> 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").
Indeed, [1] shows the inconsistency. The argument for JSON is that
"export-size-str" is not designed for machine parsing (it already
makes approximations, so outputting "unavailable" is no worse for a
human to parse than "1G", while machines should be parsing
"export-size" if present).
Additionally, having the last row in JSON be unconditionally printed
makes handling trailing commas (which are not allowed in JSON) easier
to worry about: earlier lines don't have to worry about whether to
conditionally suppress their comma. So the fact that the last line is
intended for humans rather than machines makes it easy for us to keep
to our trailing comma hack, as long as we put something there.
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.
But now that you've brought it up, I'm leaning the other direction.
When you omit --json, the results are ALL intended for human
consumption. Rereading at [2], we base a decision on human_size_flag
on whether we output:
export-size: 512
vs.
export-size: 1024 (1K)
whereas in --json, we would have:
"export-size":512, "export-size-str":"512"
vs.
"export-size":1024, "export-size-str":"1K"
So back to your question, I do think it would be nicer to the user to
explicitly let them knwo that size was unavailable; in the
machine-specific context, there is no number to parse (so we omit
"export-size"), but in the human context, we can just give a nice
string:
export-size: unavailable
vs.
"export-size-str":"unavailable"
Done with the following patch squashed in:
diff --git i/info/show.c w/info/show.c
index 3d80545e..6aeffb54 100644
--- i/info/show.c
+++ w/info/show.c
@@ -47,7 +47,7 @@ show_one_export (struct nbd_handle *nbd, const char *desc,
{
int64_t i, size;
char size_str[HUMAN_SIZE_LONGEST] = "unavailable";
- bool human_size_flag;
+ bool human_size_flag = false;
char *export_name = NULL;
char *export_desc = NULL;
char *content = NULL;
@@ -89,9 +89,8 @@ show_one_export (struct nbd_handle *nbd, const char *desc,
return false;
}
size = nbd_get_size (nbd);
- if (size >= 0) {
+ if (size >= 0)
human_size (size_str, size, &human_size_flag);
- }
if (uri_is_meaningful ())
uri = nbd_get_uri (nbd);
@@ -144,6 +143,8 @@ show_one_export (struct nbd_handle *nbd, const char *desc,
else
fprintf (fp, "\texport-size: %" PRIi64 "\n", size);
}
+ else
+ fprintf (fp, "\texport-size: %s\n", size_str);
if (content)
fprintf (fp, "\tcontent: %s\n", content);
if (uri)
If OTOH the behavior is intentional, then
Reviewed-by: Laszlo Ersek <lersek(a)redhat.com>
--
Eric Blake, Principal Software Engineer
Red Hat, Inc.
Virtualization:
qemu.org |
libguestfs.org