Eric Blake
2023-Sep-21 20:58 UTC
[Libguestfs] [libnbd PATCH v2 6/6] info: Tolerate missing size
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)
--
2.41.0
Richard W.M. Jones
2023-Sep-21 21:28 UTC
[Libguestfs] [libnbd PATCH v2 6/6] info: Tolerate missing size
On Thu, Sep 21, 2023 at 03:58:05PM -0500, 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)Reviewed-by: Richard W.M. Jones <rjones at redhat.com> -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com virt-builder quickly builds VMs from scratch http://libguestfs.org/virt-builder.1.html
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>