Re: [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
> > ---
> > 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, _size_flag);
> >}
> >
> > - human_size (size_str, size, _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
Re: [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
> ---
> 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, _size_flag);
>}
>
> - human_size (size_str, size, _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
___
Libguestfs mailing list
Libguestfs@redhat.com
https://listman.redhat.com/mailman/listinfo/libguestfs
Re: [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
> ---
> 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, _size_flag);
>}
>
> - human_size (size_str, size, _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
--
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
___
Libguestfs mailing list
Libguestfs@redhat.com
https://listman.redhat.com/mailman/listinfo/libguestfs
[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
---
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, _size_flag);
}
- human_size (size_str, size, _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
___
Libguestfs mailing list
Libguestfs@redhat.com
https://listman.redhat.com/mailman/listinfo/libguestfs
