Richard W.M. Jones
2020-Oct-06 16:54 UTC
[Libguestfs] [PATCH libnbd] info: Write output atomically.
If the server fails, nbdinfo can write partial output before the error
message (albeit on different channels). Here is an example:
$ nbdkit eval open='echo EIO fail >&2; exit 1' --run
'nbdinfo --json "$uri"'
{
"protocol": "newstyle-fixed",
"TLS": false,
nbdkit: eval[1]: error: /tmp/nbdkitii3pZW/open: fail
nbdkit: eval[1]: error: /tmp/nbdkitii3pZW/open: fail
nbdinfo: nbd_opt_go: server replied with error to opt_go request: No such file
or directory
Notice the partial JSON document, and the error message from nbdinfo.
With this commit nbdinfo tries to produce either the complete output
or the error message.
---
info/Makefile.am | 1 +
info/info-atomic-output.sh | 32 ++++++
info/nbdinfo.c | 194 ++++++++++++++++++++++---------------
3 files changed, 147 insertions(+), 80 deletions(-)
diff --git a/info/Makefile.am b/info/Makefile.am
index 9557f59..f0edec6 100644
--- a/info/Makefile.am
+++ b/info/Makefile.am
@@ -32,6 +32,7 @@ info_sh_files = \
info-map-base-allocation.sh \
info-map-base-allocation-json.sh \
info-map-qemu-dirty-bitmap.sh \
+ info-atomic-output.sh \
$(NULL)
EXTRA_DIST = \
diff --git a/info/info-atomic-output.sh b/info/info-atomic-output.sh
new file mode 100755
index 0000000..11b2ab3
--- /dev/null
+++ b/info/info-atomic-output.sh
@@ -0,0 +1,32 @@
+#!/usr/bin/env bash
+# nbd client library in userspace
+# Copyright (C) 2020 Red Hat Inc.
+#
+# This library is free software; you can redistribute it and/or
+# modify it under the terms of the GNU Lesser General Public
+# License as published by the Free Software Foundation; either
+# version 2 of the License, or (at your option) any later version.
+#
+# This library is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
+# Lesser General Public License for more details.
+#
+# You should have received a copy of the GNU Lesser General Public
+# License along with this library; if not, write to the Free Software
+# Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA
+
+. ../tests/functions.sh
+
+set -e
+set -x
+
+requires nbdkit --version
+requires nbdkit eval --version
+
+out=info-atomic-output.out
+cleanup_fn rm -f $out
+
+nbdkit -U - eval open='echo EIO fail >&2; exit 1' \
+ --run '$VG nbdinfo --size
"nbd+unix:///?socket=$unixsocket"' > $out ||:
+test ! -s $out
diff --git a/info/nbdinfo.c b/info/nbdinfo.c
index 31e2508..24ec129 100644
--- a/info/nbdinfo.c
+++ b/info/nbdinfo.c
@@ -32,6 +32,7 @@
#include <libnbd.h>
static const char *progname;
+static FILE *fp;
static bool list_all = false;
static bool probe_content, content_flag, no_content_flag;
static bool json_output = false;
@@ -121,6 +122,8 @@ main (int argc, char *argv[])
int64_t size;
const char *protocol;
int tls_negotiated;
+ char *output = NULL;
+ size_t output_len = 0;
progname = argv[0];
@@ -207,6 +210,17 @@ main (int argc, char *argv[])
if (map)
probe_content = false;
+ /* Try to write output atomically. We spool output into a
+ * memstream, pointed to by fp, and write it all at once at the end.
+ * On error nothing should be printed on stdout.
+ */
+ fp = open_memstream (&output, &output_len);
+ if (fp == NULL) {
+ fprintf (stderr, "%s: ", progname);
+ perror ("open_memstream");
+ exit (EXIT_FAILURE);
+ }
+
/* Open the NBD side. */
nbd = nbd_create ();
if (nbd == NULL) {
@@ -250,7 +264,7 @@ main (int argc, char *argv[])
exit (EXIT_FAILURE);
}
- printf ("%" PRIi64 "\n", size);
+ fprintf (fp, "%" PRIi64 "\n", size);
}
else if (map) { /* --map (!list_all) */
uint64_t offset, prev_offset;
@@ -269,7 +283,7 @@ main (int argc, char *argv[])
exit (EXIT_FAILURE);
}
- if (json_output) printf ("[\n");
+ if (json_output) fprintf (fp, "[\n");
for (offset = 0; offset < size;) {
prev_offset = offset;
if (nbd_block_status (nbd, size - offset, offset,
@@ -288,7 +302,7 @@ main (int argc, char *argv[])
exit (EXIT_FAILURE);
}
}
- if (json_output) printf ("\n]\n");
+ if (json_output) fprintf (fp, "\n]\n");
}
else { /* not --size or --map */
/* Print per-connection fields. */
@@ -297,22 +311,22 @@ main (int argc, char *argv[])
if (!json_output) {
if (protocol) {
- printf ("protocol: %s", protocol);
+ fprintf (fp, "protocol: %s", protocol);
if (tls_negotiated >= 0)
- printf (" %s TLS", tls_negotiated ? "with" :
"without");
- printf ("\n");
+ fprintf (fp, " %s TLS", tls_negotiated ? "with" :
"without");
+ fprintf (fp, "\n");
}
}
else {
- printf ("{\n");
+ fprintf (fp, "{\n");
if (protocol) {
- printf ("\"protocol\": ");
+ fprintf (fp, "\"protocol\": ");
print_json_string (protocol);
- printf (",\n");
+ fprintf (fp, ",\n");
}
if (tls_negotiated >= 0)
- printf ("\"TLS\": %s,\n", tls_negotiated ?
"true" : "false");
+ fprintf (fp, "\"TLS\": %s,\n", tls_negotiated ?
"true" : "false");
}
if (!list_all)
@@ -321,7 +335,7 @@ main (int argc, char *argv[])
list_all_exports (nbd, argv[optind]);
if (json_output)
- printf ("}\n");
+ fprintf (fp, "}\n");
}
for (i = 0; i < export_list.len; i++) {
@@ -333,6 +347,19 @@ main (int argc, char *argv[])
nbd_opt_abort (nbd);
nbd_shutdown (nbd, 0);
nbd_close (nbd);
+
+ /* Close the output stream and copy it to the real stdout. */
+ if (fclose (fp) == EOF) {
+ fprintf (stderr, "%s: ", progname);
+ perror ("fclose");
+ exit (EXIT_FAILURE);
+ }
+ if (puts (output) == EOF) {
+ fprintf (stderr, "%s: ", progname);
+ perror ("puts");
+ exit (EXIT_FAILURE);
+ }
+
exit (EXIT_SUCCESS);
}
@@ -451,126 +478,133 @@ list_one_export (struct nbd_handle *nbd, const char
*desc,
content = get_content (nbd, size);
if (!json_output) {
- printf ("export=");
+ fprintf (fp, "export=");
/* Might as well use the JSON function to get an escaped string here ... */
print_json_string (export_name);
- printf (":\n");
+ fprintf (fp, ":\n");
if (desc && *desc)
- printf ("\tdescription: %s\n", desc);
- printf ("\texport-size: %" PRIi64 "\n", size);
+ fprintf (fp, "\tdescription: %s\n", desc);
+ fprintf (fp, "\texport-size: %" PRIi64 "\n", size);
if (content)
- printf ("\tcontent: %s\n", content);
+ fprintf (fp, "\tcontent: %s\n", content);
if (show_context) {
- printf ("\tcontexts:\n");
+ fprintf (fp, "\tcontexts:\n");
for (struct context_list *next = contexts; next; next = next->next)
- printf ("\t\t%s\n", next->name);
+ fprintf (fp, "\t\t%s\n", next->name);
}
if (is_rotational >= 0)
- printf ("\t%s: %s\n", "is_rotational", is_rotational
? "true" : "false");
+ fprintf (fp, "\t%s: %s\n", "is_rotational",
+ is_rotational ? "true" : "false");
if (is_read_only >= 0)
- printf ("\t%s: %s\n", "is_read_only", is_read_only ?
"true" : "false");
+ fprintf (fp, "\t%s: %s\n", "is_read_only",
+ is_read_only ? "true" : "false");
if (can_cache >= 0)
- printf ("\t%s: %s\n", "can_cache", can_cache ?
"true" : "false");
+ fprintf (fp, "\t%s: %s\n", "can_cache", can_cache ?
"true" : "false");
if (can_df >= 0)
- printf ("\t%s: %s\n", "can_df", can_df ?
"true" : "false");
+ fprintf (fp, "\t%s: %s\n", "can_df", can_df ?
"true" : "false");
if (can_fast_zero >= 0)
- printf ("\t%s: %s\n", "can_fast_zero", can_fast_zero
? "true" : "false");
+ fprintf (fp, "\t%s: %s\n", "can_fast_zero",
+ can_fast_zero ? "true" : "false");
if (can_flush >= 0)
- printf ("\t%s: %s\n", "can_flush", can_flush ?
"true" : "false");
+ fprintf (fp, "\t%s: %s\n", "can_flush", can_flush ?
"true" : "false");
if (can_fua >= 0)
- printf ("\t%s: %s\n", "can_fua", can_fua ?
"true" : "false");
+ fprintf (fp, "\t%s: %s\n", "can_fua", can_fua ?
"true" : "false");
if (can_multi_conn >= 0)
- printf ("\t%s: %s\n", "can_multi_conn",
can_multi_conn ? "true" : "false");
+ fprintf (fp, "\t%s: %s\n", "can_multi_conn",
+ can_multi_conn ? "true" : "false");
if (can_trim >= 0)
- printf ("\t%s: %s\n", "can_trim", can_trim ?
"true" : "false");
+ fprintf (fp, "\t%s: %s\n", "can_trim", can_trim ?
"true" : "false");
if (can_zero >= 0)
- printf ("\t%s: %s\n", "can_zero", can_zero ?
"true" : "false");
+ fprintf (fp, "\t%s: %s\n", "can_zero", can_zero ?
"true" : "false");
if (block_minimum > 0)
- printf ("\t%s: %" PRId64 "\n",
"block_size_minimum", block_minimum);
+ fprintf (fp, "\t%s: %" PRId64 "\n",
"block_size_minimum", block_minimum);
if (block_preferred > 0)
- printf ("\t%s: %" PRId64 "\n",
"block_size_preferred", block_preferred);
+ fprintf (fp, "\t%s: %" PRId64 "\n",
"block_size_preferred",
+ block_preferred);
if (block_maximum > 0)
- printf ("\t%s: %" PRId64 "\n",
"block_size_maximum", block_maximum);
+ fprintf (fp, "\t%s: %" PRId64 "\n",
"block_size_maximum", block_maximum);
}
else {
if (first)
- printf ("\"exports\": [\n");
- printf ("\t{\n");
+ fprintf (fp, "\"exports\": [\n");
+ fprintf (fp, "\t{\n");
- printf ("\t\"export-name\": ");
+ fprintf (fp, "\t\"export-name\": ");
print_json_string (export_name);
- printf (",\n");
+ fprintf (fp, ",\n");
if (desc && *desc) {
- printf ("\t\"description\": ");
+ fprintf (fp, "\t\"description\": ");
print_json_string (desc);
- printf (",\n");
+ fprintf (fp, ",\n");
}
if (content) {
- printf ("\t\"content\": ");
+ fprintf (fp, "\t\"content\": ");
print_json_string (content);
- printf (",\n");
+ fprintf (fp, ",\n");
}
if (show_context) {
- printf ("\t\"contexts\": [\n");
+ fprintf (fp, "\t\"contexts\": [\n");
for (struct context_list *next = contexts; next; next = next->next) {
- printf ("\t\t");
+ fprintf (fp, "\t\t");
print_json_string (next->name);
if (next->next)
- putchar(',');
- putchar('\n');
+ fputc (',', fp);
+ fputc ('\n', fp);
}
- printf ("\t],\n");
+ fprintf (fp, "\t],\n");
}
if (is_rotational >= 0)
- printf ("\t\"%s\": %s,\n",
+ fprintf (fp, "\t\"%s\": %s,\n",
"is_rotational", is_rotational ? "true" :
"false");
if (is_read_only >= 0)
- printf ("\t\"%s\": %s,\n",
+ fprintf (fp, "\t\"%s\": %s,\n",
"is_read_only", is_read_only ? "true" :
"false");
if (can_cache >= 0)
- printf ("\t\"%s\": %s,\n",
+ fprintf (fp, "\t\"%s\": %s,\n",
"can_cache", can_cache ? "true" :
"false");
if (can_df >= 0)
- printf ("\t\"%s\": %s,\n",
+ fprintf (fp, "\t\"%s\": %s,\n",
"can_df", can_df ? "true" :
"false");
if (can_fast_zero >= 0)
- printf ("\t\"%s\": %s,\n",
+ fprintf (fp, "\t\"%s\": %s,\n",
"can_fast_zero", can_fast_zero ? "true" :
"false");
if (can_flush >= 0)
- printf ("\t\"%s\": %s,\n",
+ fprintf (fp, "\t\"%s\": %s,\n",
"can_flush", can_flush ? "true" :
"false");
if (can_fua >= 0)
- printf ("\t\"%s\": %s,\n",
+ fprintf (fp, "\t\"%s\": %s,\n",
"can_fua", can_fua ? "true" :
"false");
if (can_multi_conn >= 0)
- printf ("\t\"%s\": %s,\n",
+ fprintf (fp, "\t\"%s\": %s,\n",
"can_multi_conn", can_multi_conn ? "true" :
"false");
if (can_trim >= 0)
- printf ("\t\"%s\": %s,\n",
+ fprintf (fp, "\t\"%s\": %s,\n",
"can_trim", can_trim ? "true" :
"false");
if (can_zero >= 0)
- printf ("\t\"%s\": %s,\n",
+ fprintf (fp, "\t\"%s\": %s,\n",
"can_zero", can_zero ? "true" :
"false");
if (block_minimum > 0)
- printf ("\t\"%s\": %" PRId64 ",\n",
"block_size_minimum", block_minimum);
+ fprintf (fp, "\t\"%s\": %" PRId64 ",\n",
"block_size_minimum",
+ block_minimum);
if (block_preferred > 0)
- printf ("\t\"%s\": %" PRId64 ",\n",
"block_size_preferred",
+ fprintf (fp, "\t\"%s\": %" PRId64 ",\n",
"block_size_preferred",
block_preferred);
if (block_maximum > 0)
- printf ("\t\"%s\": %" PRId64 ",\n",
"block_size_maximum", block_maximum);
+ fprintf (fp, "\t\"%s\": %" PRId64 ",\n",
"block_size_maximum",
+ block_maximum);
/* Put this one at the end because of the stupid comma thing in JSON. */
- printf ("\t\"export-size\": %" PRIi64 "\n",
size);
+ fprintf (fp, "\t\"export-size\": %" PRIi64
"\n", size);
if (last)
- printf ("\t} ]\n");
+ fprintf (fp, "\t} ]\n");
else
- printf ("\t},\n");
+ fprintf (fp, "\t},\n");
}
while (contexts) {
@@ -590,7 +624,7 @@ list_all_exports (struct nbd_handle *nbd1, const char *uri)
size_t i;
if (export_list.len == 0 && json_output)
- printf ("\"exports\": []\n");
+ fprintf (fp, "\"exports\": []\n");
for (i = 0; i < export_list.len; ++i) {
const char *name = export_list.names[i];
@@ -635,22 +669,22 @@ list_all_exports (struct nbd_handle *nbd1, const char
*uri)
static void
print_json_string (const char *s)
{
- putc ('"', stdout);
+ fputc ('"', fp);
for (; *s; s++) {
switch (*s) {
case '\\':
case '"':
- putc ('\\', stdout);
- putc (*s, stdout);
+ fputc ('\\', fp);
+ fputc (*s, fp);
break;
default:
if (*s < ' ')
- printf ("\\u%04x", *s);
+ fprintf (fp, "\\u%04x", *s);
else
- putc (*s, stdout);
+ fputc (*s, fp);
}
}
- putc ('"', stdout);
+ fputc ('"', fp);
}
/* Run the file(1) command on the first part of the export and save
@@ -768,27 +802,27 @@ extent_callback (void *user_data, const char *metacontext,
const char *descr = extent_description (map, entries[i+1]);
if (!json_output) {
- printf ("%10" PRIu64 " "
- "%10" PRIu32 " "
- "%3" PRIu32,
- offset, entries[i], entries[i+1]);
+ fprintf (fp, "%10" PRIu64 " "
+ "%10" PRIu32 " "
+ "%3" PRIu32,
+ offset, entries[i], entries[i+1]);
if (descr)
- printf (" %s", descr);
- printf ("\n");
+ fprintf (fp, " %s", descr);
+ fprintf (fp, "\n");
}
else {
if (comma)
- printf (",\n");
+ fprintf (fp, ",\n");
- printf ("{ \"offset\": %" PRIu64 ", "
- "\"length\": %" PRIu32 ", "
- "\"type\": %" PRIu32,
- offset, entries[i], entries[i+1]);
+ fprintf (fp, "{ \"offset\": %" PRIu64 ", "
+ "\"length\": %" PRIu32 ", "
+ "\"type\": %" PRIu32,
+ offset, entries[i], entries[i+1]);
if (descr) {
- printf (", \"description\": ");
+ fprintf (fp, ", \"description\": ");
print_json_string (descr);
}
- printf ("}");
+ fprintf (fp, "}");
comma = true;
}
--
2.27.0
Eric Blake
2020-Oct-06 18:01 UTC
Re: [Libguestfs] [PATCH libnbd] info: Write output atomically.
On 10/6/20 11:54 AM, Richard W.M. Jones wrote:> If the server fails, nbdinfo can write partial output before the error > message (albeit on different channels). Here is an example: > > $ nbdkit eval open='echo EIO fail >&2; exit 1' --run 'nbdinfo --json "$uri"' > { > "protocol": "newstyle-fixed", > "TLS": false, > nbdkit: eval[1]: error: /tmp/nbdkitii3pZW/open: fail > nbdkit: eval[1]: error: /tmp/nbdkitii3pZW/open: fail > nbdinfo: nbd_opt_go: server replied with error to opt_go request: No such file or directory > > Notice the partial JSON document, and the error message from nbdinfo. > > With this commit nbdinfo tries to produce either the complete output > or the error message. > --- > info/Makefile.am | 1 + > info/info-atomic-output.sh | 32 ++++++ > info/nbdinfo.c | 194 ++++++++++++++++++++++--------------- > 3 files changed, 147 insertions(+), 80 deletions(-)ACK. Mostly mechanical, and memstreams make this nice.> +++ b/info/info-atomic-output.sh > @@ -0,0 +1,32 @@> +. ../tests/functions.sh > + > +set -e > +set -x > + > +requires nbdkit --version > +requires nbdkit eval --versionYou could simplify this: if the second line passes, the first line is implied, therefore, only the second is needed.> + > +out=info-atomic-output.out > +cleanup_fn rm -f $out > + > +nbdkit -U - eval open='echo EIO fail >&2; exit 1' \ > + --run '$VG nbdinfo --size "nbd+unix:///?socket=$unixsocket"' > $out ||:nbdkit added $uri in 1.14, and the eval plugin in 1.18. You could shorten this to --run '$VG nbdino --size "$uri"'. (We still use the longhand elsewhere because of older RHEL having older nbdkit, but this will already be skipped on those setups). -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org
Richard W.M. Jones
2020-Oct-06 18:24 UTC
Re: [Libguestfs] [PATCH libnbd] info: Write output atomically.
On Tue, Oct 06, 2020 at 01:01:49PM -0500, Eric Blake wrote:> On 10/6/20 11:54 AM, Richard W.M. Jones wrote: > > If the server fails, nbdinfo can write partial output before the error > > message (albeit on different channels). Here is an example: > > > > $ nbdkit eval open='echo EIO fail >&2; exit 1' --run 'nbdinfo --json "$uri"' > > { > > "protocol": "newstyle-fixed", > > "TLS": false, > > nbdkit: eval[1]: error: /tmp/nbdkitii3pZW/open: fail > > nbdkit: eval[1]: error: /tmp/nbdkitii3pZW/open: fail > > nbdinfo: nbd_opt_go: server replied with error to opt_go request: No such file or directory > > > > Notice the partial JSON document, and the error message from nbdinfo. > > > > With this commit nbdinfo tries to produce either the complete output > > or the error message. > > --- > > info/Makefile.am | 1 + > > info/info-atomic-output.sh | 32 ++++++ > > info/nbdinfo.c | 194 ++++++++++++++++++++++--------------- > > 3 files changed, 147 insertions(+), 80 deletions(-) > > ACK. Mostly mechanical, and memstreams make this nice. > > > > +++ b/info/info-atomic-output.sh > > @@ -0,0 +1,32 @@ > > > +. ../tests/functions.sh > > + > > +set -e > > +set -x > > + > > +requires nbdkit --version > > +requires nbdkit eval --version > > You could simplify this: if the second line passes, the first line is > implied, therefore, only the second is needed. > > > + > > +out=info-atomic-output.out > > +cleanup_fn rm -f $out > > + > > +nbdkit -U - eval open='echo EIO fail >&2; exit 1' \ > > + --run '$VG nbdinfo --size "nbd+unix:///?socket=$unixsocket"' > $out ||: > > nbdkit added $uri in 1.14, and the eval plugin in 1.18. You could > shorten this to --run '$VG nbdino --size "$uri"'. (We still use the > longhand elsewhere because of older RHEL having older nbdkit, but this > will already be skipped on those setups).Yes, good point. For the record RHEL 7 has nbdkit 1.8 (forever now), but of course no eval plugin. Thanks, Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com virt-top is 'top' for virtual machines. Tiny program with many powerful monitoring features, net stats, disk stats, logging, etc. http://people.redhat.com/~rjones/virt-top
Possibly Parallel Threads
- [PATCH libnbd] info: Write output atomically.
- [libnbd PATCH 3/3] api: Add nbd_opt_list_meta_context
- [libnbd PATCH v4 20/25] generator: Actually request extended headers
- [libnbd PATCH 2/2] info: Expose description in list mode
- [libnbd PATCH v2 11/13] api: Add nbd_aio_opt_list