Laszlo Ersek
2023-Sep-22 12:19 UTC
[Libguestfs] [libnbd PATCH v2 6/6] info: Tolerate missing size
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>
Eric Blake
2023-Sep-22 14:03 UTC
[Libguestfs] [libnbd PATCH v2 6/6] info: Tolerate missing size
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 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;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 at redhat.com> >-- Eric Blake, Principal Software Engineer Red Hat, Inc. Virtualization: qemu.org | libguestfs.org