[nbdkit] Access export name from plugins
by Richard W.M. Jones
Of course at the moment nbdkit parses the NBD export name option but
doesn't really do anything with it (except logging it).
I wonder if we should make this available to plugins, in case they
wish to serve different content to different clients based on the
export name. Note I'm not suggesting that we use this feature in any
existing plugins.
If we wanted to do this there seem like two possible ways to do it:
(1) Add a call, like nbdkit_export_name, which plugins could call from
any connected method to get the current export name, eg:
static void *
myplugin_open (int readonly)
{
const char *export = nbdkit_export_name ();
... Do something based on the export name ...
}
The implementation of this is straightforward. It simply reads the
exportname global from the server and returns it. It also doesn't
change the existing API at all.
(2) A better way might be to bump the API version to 3 and that would
allow us to change the open() callback:
static void *
myplugin_open (int readonly, const char *exportname)
{
... Do something based on the export name ...
}
This is cleaner but is obviously a larger change. Also if we were
going to bump the API version I'd want to at the same time do all the
other things we are planning for API 3, and that potentially makes it
a much bigger deal:
https://github.com/libguestfs/nbdkit/blob/b485ade71464fc351828e2fcce54464...
It has the advantage of making it easier to access the export name
from sh plugins, since those don't have access to nbdkit_* API calls
(or at least we haven't thought about how we would do that).
Thoughts?
Rich.
--
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
libguestfs lets you edit virtual machines. Supports shell scripting,
bindings from many languages. http://libguestfs.org
4 years, 8 months
[libnbd PATCH] maint: Update reference to license info
by Eric Blake
Our README file claims that license info is in LICENSE, but we did not
have a file by that name in the tarball. At least we did correctly
ship COPYING.LIB since the library is LGPLv2+.
---
The LGPL requires that the user also receive a copy of the GPL, since
anyone can upgrade their copy from LGPL to GPL. Does that mean we
should ship a copy of COPYING alongside COPYING.LIB?
README | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/README b/README
index c591f4a..d6d1704 100644
--- a/README
+++ b/README
@@ -29,7 +29,8 @@ LICENSE
The software is copyright © Red Hat Inc. and licensed under the GNU
Lesser General Public License version 2 or above (LGPLv2+). See
-‘LICENSE’ for details.
+the file ‘COPYING.LIB’ for details. The examples are under a very
+liberal license.
BUILDING FROM SOURCE
--
2.21.0
4 years, 8 months
[PATCH] drives: Typo fix
by Eric Blake
Favor 'atomically' over 'atomicly'.
Signed-off-by: Eric Blake <eblake(a)redhat.com>
---
I don't have push rights in libguestfs (yet?)
Several .po files have the same typo, but I think they get updated
mechanically, so I didn't bother to change them.
lib/drives.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/lib/drives.c b/lib/drives.c
index 82ef30093..5a8d29ab4 100644
--- a/lib/drives.c
+++ b/lib/drives.c
@@ -1064,7 +1064,7 @@ guestfs_impl_remove_drive (guestfs_h *g, const char *label)
/**
* Checkpoint and roll back drives, so that groups of drives can be
- * added atomicly. Only used by L<guestfs(3)/guestfs_add_domain>.
+ * added atomically. Only used by L<guestfs(3)/guestfs_add_domain>.
*/
size_t
guestfs_int_checkpoint_drives (guestfs_h *g)
--
2.21.0
4 years, 8 months
[PATCH nbdkit] Ban use of stack Variable Length Arrays (VLAs).
by Richard W.M. Jones
I'm not someone who thinks VLAs are automatically bad and unlike Linux
kernel code they can sometimes be used safely in userspace. However
for an internet exposed server there is an argument that they might
cause some kind of exploitable situation especially if the code is
compiled without other stack hardening features. Also in highly
multithreaded code with limited stack sizes (as nbdkit is on some
platforms) allowing unbounded stack allocation can be a bad idea
because it can cause a segfault.
So this commit bans them. Only when using --enable-gcc-warnings, but
upstream developers ought to be using that.
There were in fact only two places where VLAs were being used. In one
of those places (plugins/sh) removing the VLA actually made the code
better.
For interesting discussion about VLAs in the kernel see:
https://lwn.net/Articles/763253/
---
configure.ac | 2 +-
plugins/sh/sh.c | 7 +++----
server/sockets.c | 8 +++++++-
3 files changed, 11 insertions(+), 6 deletions(-)
diff --git a/configure.ac b/configure.ac
index 5842c6f..d326377 100644
--- a/configure.ac
+++ b/configure.ac
@@ -90,7 +90,7 @@ AC_ARG_ENABLE([gcc-warnings],
[gcc_warnings=no]
)
if test "x$gcc_warnings" = "xyes"; then
- WARNINGS_CFLAGS="-Wall -Wshadow -Werror"
+ WARNINGS_CFLAGS="-Wall -Wshadow -Wvla -Werror"
AC_SUBST([WARNINGS_CFLAGS])
fi
diff --git a/plugins/sh/sh.c b/plugins/sh/sh.c
index c73b08b..acb50c4 100644
--- a/plugins/sh/sh.c
+++ b/plugins/sh/sh.c
@@ -74,8 +74,7 @@ sh_load (void)
static void
sh_unload (void)
{
- const size_t tmpdir_len = strlen (tmpdir);
- char cmd[7 + tmpdir_len + 1]; /* "rm -rf " + tmpdir + \0 */
+ CLEANUP_FREE char *cmd = NULL;
/* Run the unload method. Ignore all errors. */
if (script) {
@@ -85,8 +84,8 @@ sh_unload (void)
}
/* Delete the temporary directory. Ignore all errors. */
- snprintf (cmd, sizeof cmd, "rm -rf %s", tmpdir);
- system (cmd);
+ if (asprintf (&cmd, "rm -rf %s", tmpdir) >= 0)
+ system (cmd);
free (script);
}
diff --git a/server/sockets.c b/server/sockets.c
index 26d65c6..dfaa3ea 100644
--- a/server/sockets.c
+++ b/server/sockets.c
@@ -366,10 +366,16 @@ accept_connection (int listen_sock)
static void
check_sockets_and_quit_fd (int *socks, size_t nr_socks)
{
- struct pollfd fds[nr_socks + 1];
size_t i;
int r;
+ CLEANUP_FREE struct pollfd *fds =
+ malloc (sizeof (struct pollfd) * (nr_socks+1));
+ if (fds == NULL) {
+ perror ("malloc");
+ exit (EXIT_FAILURE);
+ }
+
for (i = 0; i < nr_socks; ++i) {
fds[i].fd = socks[i];
fds[i].events = POLLIN;
--
2.23.0
4 years, 8 months
[PATCH libnbd] configure: Ban use of Variable Length Arrays (VLAs).
by Richard W.M. Jones
Since we don't know much about the calling environment, which might
have a limited stack, might be taking input from untrusted sources, or
we might not have other stack protections enabled, it's best to be
cautious about using unbounded stack allocations.
We're not in fact using them in libnbd, but this change prevents them
from being added in future.
---
configure.ac | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/configure.ac b/configure.ac
index 6ea3197..0332d77 100644
--- a/configure.ac
+++ b/configure.ac
@@ -66,7 +66,7 @@ AC_ARG_ENABLE([gcc-warnings],
[gcc_warnings=no]
)
if test "x$gcc_warnings" = "xyes"; then
- WARNINGS_CFLAGS="-Wall -Werror"
+ WARNINGS_CFLAGS="-Wall -Wvla -Werror"
AC_SUBST([WARNINGS_CFLAGS])
fi
--
2.23.0
4 years, 8 months
[libnbd PATCH] api: Add way to avoid structured replies
by Eric Blake
We want to default to requesting structured replies, whether or not
that request will be honored (it's essential for efficient sparse file
reads and the DF flag for structured pread, as well as for meta
context support even if we do not request a default meta context).
However, for integration testing, it can be nice to easily request a
client that does not request structured replies.
We can test this by reusing our eflags test. Note that nbdkit does
not provide a 'can_df' callback in the sh script (so our key=value
override is silently ignored), rather, we control whether nbdkit
advertises df based on whether we request structured replies.
---
I'm open to renaming the API to the shorter 'nbd_set_request_sr' if
the existing name choice seems too long.
This is a counterpart to the recent addition of --no-sr in nbdkit 1.14
for server-side disable; but by doing it in the client side, we can
test lack of structured replies even while still using nbdkit 1.12.x.
lib/internal.h | 1 +
generator/generator | 30 +++++++++++++++++++
.../states-newstyle-opt-structured-reply.c | 5 ++++
lib/handle.c | 15 ++++++++++
tests/Makefile.am | 18 +++++++++++
tests/eflags.c | 6 ++++
TODO | 2 +-
7 files changed, 76 insertions(+), 1 deletion(-)
diff --git a/lib/internal.h b/lib/internal.h
index 581777a..a48edff 100644
--- a/lib/internal.h
+++ b/lib/internal.h
@@ -73,6 +73,7 @@ struct nbd_handle {
char *tls_psk_file; /* PSK filename, NULL = no PSK */
/* Desired metadata contexts. */
+ bool request_sr;
char **request_meta_contexts;
/* Global flags from the server. */
diff --git a/generator/generator b/generator/generator
index 1cc5c19..5c32afa 100755
--- a/generator/generator
+++ b/generator/generator
@@ -1243,6 +1243,36 @@ Get the current TLS PSK filename.";
};
*)
+ "set_request_structured_replies", {
+ default_call with
+ args = [Bool "request"]; ret = RErr;
+ permitted_states = [ Created ];
+ first_version = (1, 2);
+ shortdesc = "control use of structured replies";
+ longdesc = "\
+By default, libnbd tries to negotiate structured replies with the
+server, as this protocol extension must be in use before
+C<nbd_can_meta_context> or C<nbd_can_df> can return true. However,
+for integration testing, it can be useful to clear this flag
+rather than find a way to alter the server to fail the negotiation
+request.";
+ see_also = ["L<nbd_get_request_structured_replies(3)>";
+ "L<nbd_can_meta_context(3)>"; "L<nbd_can_df(3)>"];
+ };
+
+ "get_request_structured_replies", {
+ default_call with
+ args = []; ret = RBool;
+ first_version = (1, 2);
+ shortdesc = "see if structured replies are attempted";
+ longdesc = "\
+Return the state of the request structured replies flag on this
+handle. Note that this only reports whether the client attempts
+to negotiate structured replies, and not whether the server was
+able to honor that request";
+ see_also = ["L<nbd_set_request_structured_replies(3)>"];
+ };
+
"add_meta_context", {
default_call with
args = [ String "name" ]; ret = RErr;
diff --git a/generator/states-newstyle-opt-structured-reply.c b/generator/states-newstyle-opt-structured-reply.c
index d932248..415f7e0 100644
--- a/generator/states-newstyle-opt-structured-reply.c
+++ b/generator/states-newstyle-opt-structured-reply.c
@@ -20,6 +20,11 @@
/* STATE MACHINE */ {
NEWSTYLE.OPT_STRUCTURED_REPLY.START:
+ if (!h->request_sr) {
+ SET_NEXT_STATE (%^OPT_SET_META_CONTEXT.START);
+ return 0;
+ }
+
h->sbuf.option.version = htobe64 (NBD_NEW_VERSION);
h->sbuf.option.option = htobe32 (NBD_OPT_STRUCTURED_REPLY);
h->sbuf.option.optlen = htobe32 (0);
diff --git a/lib/handle.c b/lib/handle.c
index f8cc83a..c23ef01 100644
--- a/lib/handle.c
+++ b/lib/handle.c
@@ -63,6 +63,7 @@ nbd_create (void)
h->unique = 1;
h->tls_verify_peer = true;
+ h->request_sr = true;
s = getenv ("LIBNBD_DEBUG");
h->debug = s && strcmp (s, "1") == 0;
@@ -242,6 +243,20 @@ nbd_unlocked_add_meta_context (struct nbd_handle *h, const char *name)
return 0;
}
+int
+nbd_unlocked_set_request_structured_replies (struct nbd_handle *h,
+ bool request)
+{
+ h->request_sr = request;
+ return 0;
+}
+
+int
+nbd_unlocked_get_request_structured_replies (struct nbd_handle *h)
+{
+ return h->request_sr;
+}
+
const char *
nbd_unlocked_get_package_name (struct nbd_handle *h)
{
diff --git a/tests/Makefile.am b/tests/Makefile.am
index a7bc1b5..41b620d 100644
--- a/tests/Makefile.am
+++ b/tests/Makefile.am
@@ -147,6 +147,8 @@ check_PROGRAMS += \
can-not-zero-flag \
can-fast-zero-flag \
can-not-fast-zero-flag \
+ can-df-flag \
+ can-not-df-flag \
can-multi-conn-flag \
can-not-multi-conn-flag \
can-cache-flag \
@@ -181,6 +183,8 @@ TESTS += \
can-not-zero-flag \
can-fast-zero-flag \
can-not-fast-zero-flag \
+ can-df-flag \
+ can-not-df-flag \
can-multi-conn-flag \
can-not-multi-conn-flag \
can-cache-flag \
@@ -309,6 +313,20 @@ can_not_fast_zero_flag_CPPFLAGS = \
can_not_fast_zero_flag_CFLAGS = $(WARNINGS_CFLAGS)
can_not_fast_zero_flag_LDADD = $(top_builddir)/lib/libnbd.la
+can_df_flag_SOURCES = eflags.c
+can_df_flag_CPPFLAGS = \
+ -I$(top_srcdir)/include -Dflag=can_df \
+ $(NULL)
+can_df_flag_CFLAGS = $(WARNINGS_CFLAGS)
+can_df_flag_LDADD = $(top_builddir)/lib/libnbd.la
+
+can_not_df_flag_SOURCES = eflags.c
+can_not_df_flag_CPPFLAGS = \
+ -I$(top_srcdir)/include -Dflag=can_df -Dvalue=false -Dno_sr \
+ $(NULL)
+can_not_df_flag_CFLAGS = $(WARNINGS_CFLAGS)
+can_not_df_flag_LDADD = $(top_builddir)/lib/libnbd.la
+
can_multi_conn_flag_SOURCES = eflags.c
can_multi_conn_flag_CPPFLAGS = \
-I$(top_srcdir)/include -Dflag=can_multi_conn \
diff --git a/tests/eflags.c b/tests/eflags.c
index 675802a..afff3a9 100644
--- a/tests/eflags.c
+++ b/tests/eflags.c
@@ -84,6 +84,12 @@ main (int argc, char *argv[])
fprintf (stderr, "%s\n", nbd_get_error ());
exit (EXIT_FAILURE);
}
+#ifdef no_sr
+ if (nbd_set_request_structured_replies (nbd, false) == -1) {
+ fprintf (stderr, "%s\n", nbd_get_error ());
+ exit (EXIT_FAILURE);
+ }
+#endif
if (nbd_connect_command (nbd, args) == -1) {
fprintf (stderr, "%s\n", nbd_get_error ());
exit (EXIT_FAILURE);
diff --git a/TODO b/TODO
index 34bb983..642d39f 100644
--- a/TODO
+++ b/TODO
@@ -48,7 +48,7 @@ Suggested API improvements:
- support PKCS11 URIs (RFC 7512)
Easier server implementation testing:
- - a way to disable requesting structured replies
+ - a way to force NBD_OPT_EXPORT_NAME over NBD_OPT_GO
- a way to forcefully violate protocol (such as allowing writes to a
readonly connection, or sending a request that exceeds bounds) for
testing server reactions
--
2.21.0
4 years, 8 months
[PATCH] ocaml: Change calls to caml_named_value() to cope with const value* return.
by Richard W.M. Jones
In OCaml >= 4.09 the return value pointer of caml_named_value is
declared const.
Based on Pino Toscano's original patch to ocaml-augeas.
---
common/mlpcre/pcre-c.c | 3 +--
common/mltools/uri-c.c | 6 ++----
common/mlvisit/visit-c.c | 4 +---
generator/daemon.ml | 2 +-
4 files changed, 5 insertions(+), 10 deletions(-)
diff --git a/common/mlpcre/pcre-c.c b/common/mlpcre/pcre-c.c
index be054a004..d23f17e05 100644
--- a/common/mlpcre/pcre-c.c
+++ b/common/mlpcre/pcre-c.c
@@ -73,12 +73,11 @@ init (void)
static void
raise_pcre_error (const char *msg, int errcode)
{
- value *exn = caml_named_value ("PCRE.Error");
value args[2];
args[0] = caml_copy_string (msg);
args[1] = Val_int (errcode);
- caml_raise_with_args (*exn, 2, args);
+ caml_raise_with_args (*caml_named_value ("PCRE.Error"), 2, args);
}
/* Wrap and unwrap pcre regular expression handles, with a finalizer. */
diff --git a/common/mltools/uri-c.c b/common/mltools/uri-c.c
index 2a8837cd9..e03647c7b 100644
--- a/common/mltools/uri-c.c
+++ b/common/mltools/uri-c.c
@@ -46,10 +46,8 @@ guestfs_int_mllib_parse_uri (value argv /* arg value, not an array! */)
int r;
r = parse_uri (String_val (argv), &uri);
- if (r == -1) {
- value *exn = caml_named_value ("URI.Parse_failed");
- caml_raise (*exn);
- }
+ if (r == -1)
+ caml_raise (*caml_named_value ("URI.Parse_failed"));
/* Convert the struct into an OCaml tuple. */
rv = caml_alloc_tuple (5);
diff --git a/common/mlvisit/visit-c.c b/common/mlvisit/visit-c.c
index 7137c4998..201f6d762 100644
--- a/common/mlvisit/visit-c.c
+++ b/common/mlvisit/visit-c.c
@@ -53,7 +53,6 @@ value
guestfs_int_mllib_visit (value gv, value dirv, value fv)
{
CAMLparam3 (gv, dirv, fv);
- value *visit_failure_exn;
guestfs_h *g = (guestfs_h *) (intptr_t) Int64_val (gv);
struct visitor_function_wrapper_args args;
/* The dir string could move around when we call the
@@ -84,8 +83,7 @@ guestfs_int_mllib_visit (value gv, value dirv, value fv)
* already printed the error to stderr (XXX - fix), so we raise a
* generic exception.
*/
- visit_failure_exn = caml_named_value ("Visit.Failure");
- caml_raise (*visit_failure_exn);
+ caml_raise (*caml_named_value ("Visit.Failure"));
}
free (dir);
diff --git a/generator/daemon.ml b/generator/daemon.ml
index a4e136aaa..b67c4d20b 100644
--- a/generator/daemon.ml
+++ b/generator/daemon.ml
@@ -746,7 +746,7 @@ let generate_daemon_caml_stubs () =
let nr_args = List.length args_do_function in
pr "{\n";
- pr " static value *cb = NULL;\n";
+ pr " static const value *cb = NULL;\n";
pr " CAMLparam0 ();\n";
pr " CAMLlocal2 (v, retv);\n";
pr " CAMLlocalN (args, %d);\n"
--
2.23.0
4 years, 8 months