Re: [Libguestfs] [PATCH libnbd] generator: Move first_version fields to a single table.
On Thu, Sep 05, 2019 at 08:02:34AM -0500, Eric Blake wrote: > On 9/5/19 6:56 AM, Richard W.M. Jones wrote: > > See discussion in this and following messages: > > https://www.redhat.com/archives/libguestfs/2019-September/msg00020.html > > --- > > generator/generator | 131 > > 1 file changed, 121 insertions(+), 10 deletions(-) > > > > > +let first_version = [ > > + "set_debug", (1, 0); > > + "get_debug", (1, 0); > > + "set_debug_callback", (1, 0); > > + "clear_debug_callback", (1, 0); > > + "set_handle_name", (1, 0); > > + "get_handle_name", (1, 0); > > + "set_export_name", (1, 0); > > + "get_export_name", (1, 0); > > + "set_tls", (1, 0); > > + "get_tls", (1, 0); > > + "set_tls_certificates", (1, 0); > > + (*"get_tls_certificates", (1, 0);*) > > If we ever do implement this (and any other commented out for various > reasons), it won't be a 1.0 version... > > > + > > + (* Added in 1.1 development series. *) > > + "can_fast_zero", (1, 2); > > ..so maybe it's better to sink the commented future APIs to the bottom? Good point :-) I'll move these to the end of the list with a comment. Thanks, Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com virt-df lists disk usage of guests without needing to install any software inside the virtual machine. Supports Linux and Windows. http://people.redhat.com/~rjones/virt-df/ ___ Libguestfs mailing list Libguestfs@redhat.com https://www.redhat.com/mailman/listinfo/libguestfs
Re: [Libguestfs] [PATCH libnbd] generator: Move first_version fields to a single table.
On 9/5/19 6:56 AM, Richard W.M. Jones wrote: > See discussion in this and following messages: > https://www.redhat.com/archives/libguestfs/2019-September/msg00020.html > --- > generator/generator | 131 > 1 file changed, 121 insertions(+), 10 deletions(-) > > +let first_version = [ > + "set_debug", (1, 0); > + "get_debug", (1, 0); > + "set_debug_callback", (1, 0); > + "clear_debug_callback", (1, 0); > + "set_handle_name", (1, 0); > + "get_handle_name", (1, 0); > + "set_export_name", (1, 0); > + "get_export_name", (1, 0); > + "set_tls", (1, 0); > + "get_tls", (1, 0); > + "set_tls_certificates", (1, 0); > + (*"get_tls_certificates", (1, 0);*) If we ever do implement this (and any other commented out for various reasons), it won't be a 1.0 version... > + > + (* Added in 1.1 development series. *) > + "can_fast_zero", (1, 2); ..so maybe it's better to sink the commented future APIs to the bottom? -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org signature.asc Description: OpenPGP digital signature ___ Libguestfs mailing list Libguestfs@redhat.com https://www.redhat.com/mailman/listinfo/libguestfs
Re: [Libguestfs] [PATCH libnbd] generator: Move first_version fields to a single table.
On 9/5/19 6:56 AM, Richard W.M. Jones wrote: > This doesn't include Eric's new APIs, but if you push those > then I can rebase this one on top. Looks good, and my new API is in. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org signature.asc Description: OpenPGP digital signature ___ Libguestfs mailing list Libguestfs@redhat.com https://www.redhat.com/mailman/listinfo/libguestfs
Re: [Libguestfs] [PATCH libnbd] configure: Ban use of Variable Length Arrays (VLAs).
On 9/5/19 6:38 AM, Richard W.M. Jones wrote: > 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(-) > ACK > 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 > > -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org signature.asc Description: OpenPGP digital signature ___ Libguestfs mailing list Libguestfs@redhat.com https://www.redhat.com/mailman/listinfo/libguestfs
Re: [Libguestfs] [PATCH nbdkit] Ban use of stack Variable Length Arrays (VLAs).
On Thu, Sep 05, 2019 at 07:57:44AM -0500, Eric Blake wrote: > On 9/5/19 6:28 AM, Richard W.M. Jones wrote: > > 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(-) > > > +++ 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" > > I'm guessing that both gcc and clang are okay with our current list; we > may reach the point where we need to probe at configure time on which > options we can safely use, instead of merely open-coding a list, but > we'll deal with that when it breaks the build. With the proviso that I didn't actually test it under clang, it does seem to have the flag: https://clang.llvm.org/docs/DiagnosticsReference.html#wvla [...] 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 ___ Libguestfs mailing list Libguestfs@redhat.com https://www.redhat.com/mailman/listinfo/libguestfs
Re: [Libguestfs] [PATCH nbdkit] Ban use of stack Variable Length Arrays (VLAs).
On 9/5/19 6:28 AM, Richard W.M. Jones wrote:
> 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(-)
> +++ 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"
I'm guessing that both gcc and clang are okay with our current list; we
may reach the point where we need to probe at configure time on which
options we can safely use, instead of merely open-coding a list, but
we'll deal with that when it breaks the build.
> +++ 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 (, "rm -rf %s", tmpdir) >= 0)
> +system (cmd);
Safe only because our tmpdir pattern contains no shell metacharacters
(your patch does not change that fact). If we ever decided to honor
$TMPDIR (that is, creating $TMPDIR/nbdkitshXX instead of
/tmp/nbdkitshXX), then we'd need shell quoting here. But doesn't
change this patch.
> +++ 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));
This is indeed safer, but adds a malloc() in a loop. Thankfully, the
loop of accept_incoming_connections() doesn't cycle that quickly (once
per new client), so I think it's in the noise compared to pre-allocating
the array once before starting the loop.
ACK.
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org
signature.asc
Description: OpenPGP digital signature
___
Libguestfs mailing list
Libguestfs@redhat.com
https://www.redhat.com/mailman/listinfo/libguestfs
[Libguestfs] [PATCH libnbd] generator: Move first_version fields to a single table.
See discussion in this and following messages:
https://www.redhat.com/archives/libguestfs/2019-September/msg00020.html
---
generator/generator | 131
1 file changed, 121 insertions(+), 10 deletions(-)
diff --git a/generator/generator b/generator/generator
index 1cc5c19..8a3905f 100755
--- a/generator/generator
+++ b/generator/generator
@@ -845,9 +845,11 @@ type call = {
may_set_error : bool;
(* The first stable version that the symbol appeared in, for
* example (1, 2) if the symbol was added in development cycle
- * 1.1.x and thus the first stable version was 1.2
+ * 1.1.x and thus the first stable version was 1.2. This is
+ * filled in by the generator, add new calls to the first_version
+ * table instead.
*)
- first_version : int * int;
+ mutable first_version : int * int;
}
and arg =
| Bool of string (* bool *)
@@ -916,7 +918,7 @@ let default_call = { args = []; optargs = []; ret = RErr;
see_also = [];
permitted_states = [];
is_locked = true; may_set_error = true;
- first_version = (1, 0) }
+ first_version = (0, 0) }
let non_blocking_test_call_description = "\n
This call does not block, because it returns data that is saved in
the handle from the NBD protocol handshake."
@@ -1423,7 +1425,6 @@ the server does not."
default_call with
args = []; ret = RBool;
permitted_states = [ Connected; Closed ];
-first_version = (1, 2);
shortdesc = "does the server support the fast zero flag?";
longdesc = "\
Returns true if the server supports the use of the
@@ -2374,7 +2375,93 @@ Returns true if libnbd was compiled with libxml2 which
is required
to support NBD URIs, or false if not.";
see_also = ["L"; "L"];
};
+]
+(* The first stable version that the symbol appeared in, for
+ * example (1, 2) if the symbol was added in development cycle
+ * 1.1.x and thus the first stable version was 1.2.
+ *)
+let first_version = [
+ "set_debug", (1, 0);
+ "get_debug", (1, 0);
+ "set_debug_callback", (1, 0);
+ "clear_debug_callback", (1, 0);
+ "set_handle_name", (1, 0);
+ "get_handle_name", (1, 0);
+ "set_export_name", (1, 0);
+ "get_export_name", (1, 0);
+ "set_tls", (1, 0);
+ "get_tls", (1, 0);
+ "set_tls_certificates", (1, 0);
+ (*"get_tls_certificates", (1, 0);*)
+ "set_tls_verify_peer", (1, 0);
+ "get_tls_verify_peer", (1, 0);
+ "set_tls_username", (1, 0);
+ "get_tls_username", (1, 0);
+ "set_tls_psk_file", (1, 0);
+ (*"get_tls_psk_file", (1, 0);*)
+ "add_meta_context", (1, 0);
+ "connect_uri", (1, 0);
+ "connect_unix", (1, 0);
+ "connect_tcp", (1, 0);
+ "connect_command", (1, 0);
+ "is_read_only", (1, 0);
+ "can_flush", (1, 0);
+ "can_fua", (1, 0);
+ "is_rotational", (1, 0);
+ "can_trim", (1, 0);
+ "can_zero", (1, 0);
+ "can_df", (1, 0);
+ "can_multi_conn", (1, 0);
+ "can_cache", (1, 0);
+ "can_meta_context", (1, 0);
+ "get_size", (1, 0);
+ "pread", (1, 0);
+ "pread_structured", (1, 0);
+ "pwrite", (1, 0);
+ "shutdown", (1, 0);
+ "flush", (1, 0);
+ "trim", (1, 0);
+ "cache", (1, 0);
+ "zero", (1, 0);
+ "block_status", (1, 0);
+ "poll", (1, 0);
+ "aio_connect", (1, 0);
+ "aio_connect_uri", (1, 0);
+ "aio_connect_unix", (1, 0);
+ "aio_connect_tcp", (1, 0);
+ "aio_connect_command", (1, 0);
+ "aio_pread", (1, 0);
+ "aio_pread_structured", (1, 0);
+ "aio_pwrite", (1, 0);
+ "aio_disconnect", (1, 0);
+ "aio_flush", (1, 0);
+ "aio_trim", (1, 0);
+ "aio_cache", (1, 0);
+ "aio_zero", (1, 0);
+ "aio_block_status", (1, 0);
+ "aio_get_fd", (1, 0);
+ "aio_get_direction", (1, 0);
+ "aio_notify_read", (1, 0);
+ "aio_notify_write", (1, 0);
+ "aio_is_created", (1, 0);
+ "aio_is_connecting", (1, 0);
+ "aio_is_ready", (1, 0);
+ "aio_is_processing", (1, 0);
+ "aio_is_dead", (1, 0);
+ "aio_is_closed", (1, 0);
+ "aio_command_completed", (1, 0);
+ "aio_peek_command_completed", (1, 0);
+ "aio_in_flight", (1, 0);
+ "connection_state", (1, 0);
+ "get_package_name", (1, 0);
+ "get_version", (1, 0);
+ "kill_subprocess", (1, 0);
+ "supports_tls", (1, 0);
+ "supports_uri", (1, 0);
+
+ (* Added in 1.1 development series. *)
+ "can_fast_zero", (1, 2);
]
(* Constants, etc. *)
@@ -3336,13 +3423,37 @@ let () =
| _ -> ()
) handle_calls;
- (* First stable version must be 1.x where x is even. *)
+ (* first_version must be (0, 0) in handle_calls (we will modify it). *)
List.iter (
-fun (name, { first_version = (major, minor) }) ->
- if major <> 1 then
-failwithf "%s: first_version must be 1.x" name;
- if minor mod 2 <> 0 then
-failwithf "%s: first_version must refer to a stable release" name
+function
+| (_, { first_version = (0, 0) }) -> ()
+| (name, _) ->
+failwithf "%s: first_version field must not be set in handle_calls
table" name
+ ) handle_calls;
+
+ (* Check every entry in
[Libguestfs] [PATCH libnbd] generator: Move first_version fields to a single table.
This doesn't include Eric's new APIs, but if you push those then I can rebase this one on top. Rich. ___ Libguestfs mailing list Libguestfs@redhat.com https://www.redhat.com/mailman/listinfo/libguestfs
[Libguestfs] [PATCH libnbd] configure: Ban use of Variable Length Arrays (VLAs).
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 ___ Libguestfs mailing list Libguestfs@redhat.com https://www.redhat.com/mailman/listinfo/libguestfs
Re: [Libguestfs] [libnbd PATCH] api: Add way to avoid structured replies
On 9/5/19 2:40 AM, Richard W.M. Jones wrote:
> I have one comment unrelated to the patch:
>
>> + "set_request_structured_replies", {
>> +default_call with
>> +args = [Bool "request"]; ret = RErr;
>> +permitted_states = [ Created ];
>> +first_version = (1, 2);
>
> I just know that we're going to end up adding new APIs and forgetting
> to set the first_version field. There are various things we could do
> to prevent this:
>
> (1) In ‘default_call’ set first_version = (0, 0). Update all existing
> calls with first_version = (1, 0). Then add a check that
> first_version <> (0, 0). There's still a danger of copy and paste
> from an existing API, but we should be able to catch that in review.
Or maybe couple that with a declaration that maps version (1, 0) has X
signatures and version (1, 2) has Y releases, then if we detect a count
mismatch, it must be a new addition incorrectly versioned.
>
> (2) Store first_version in a separate table. Add checks to ensure the
> new table exhaustively covers all APIs. It should be obvious when
> submitting a new API that the first_version table must be updated and
> what to add here:
>
> let first_version = [
> "set_debug", (1, 0);
> ...
> "set_request_structured_replies", (1, 2);
> "get_request_structured_replies", (1, 2);
> ]
This approach also seems fine (it's a bit closer to maintaining the
.syms file by hand, but with the compiler guaranteeing that we touch
.syms for everything we add). I lean slightly towards option 2.
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org
signature.asc
Description: OpenPGP digital signature
___
Libguestfs mailing list
Libguestfs@redhat.com
https://www.redhat.com/mailman/listinfo/libguestfs
[Libguestfs] [PATCH nbdkit] Ban use of stack Variable Length Arrays (VLAs).
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 (, "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
___
Libguestfs mailing list
Libguestfs@redhat.com
https://www.redhat.com/mailman/listinfo/libguestfs
Re: [Libguestfs] [PATCH] ocaml: Change calls to caml_named_value() to cope with const value* return.
On Thursday, 5 September 2019 10:58:41 CEST Richard W.M. Jones wrote:
> 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), );
> - 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"
LGTM.
--
Pino Toscano
signature.asc
Description: This is a digitally signed message part.
___
Libguestfs mailing list
Libguestfs@redhat.com
https://www.redhat.com/mailman/listinfo/libguestfs
Re: [Libguestfs] [PATCH 1/1] ocaml: make const the return value of caml_named_value()
On Thu, Sep 05, 2019 at 10:47:42AM +0200, Pino Toscano wrote:
> With OCaml >= 4.09 caml_named_value() returns a const value *, so keep
> the constness to build also in this case.
> ---
> common/mlpcre/pcre-c.c | 2 +-
> common/mltools/uri-c.c | 2 +-
> common/mlvisit/visit-c.c | 2 +-
> generator/daemon.ml | 2 +-
> 4 files changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/common/mlpcre/pcre-c.c b/common/mlpcre/pcre-c.c
> index be054a004..62a6dd5da 100644
> --- a/common/mlpcre/pcre-c.c
> +++ b/common/mlpcre/pcre-c.c
> @@ -73,7 +73,7 @@ init (void)
> static void
> raise_pcre_error (const char *msg, int errcode)
> {
> - value *exn = caml_named_value ("PCRE.Error");
> + const value *exn = caml_named_value ("PCRE.Error");
>value args[2];
>
>args[0] = caml_copy_string (msg);
> diff --git a/common/mltools/uri-c.c b/common/mltools/uri-c.c
> index 2a8837cd9..db6fe23fc 100644
> --- a/common/mltools/uri-c.c
> +++ b/common/mltools/uri-c.c
> @@ -47,7 +47,7 @@ guestfs_int_mllib_parse_uri (value argv /* arg value, not
> an array! */)
>
>r = parse_uri (String_val (argv), );
>if (r == -1) {
> -value *exn = caml_named_value ("URI.Parse_failed");
> +const value *exn = caml_named_value ("URI.Parse_failed");
In my version I got rid of the useless local variable.
Rich.
> caml_raise (*exn);
>}
>
> diff --git a/common/mlvisit/visit-c.c b/common/mlvisit/visit-c.c
> index 7137c4998..804aabc9d 100644
> --- a/common/mlvisit/visit-c.c
> +++ b/common/mlvisit/visit-c.c
> @@ -53,7 +53,7 @@ value
> guestfs_int_mllib_visit (value gv, value dirv, value fv)
> {
>CAMLparam3 (gv, dirv, fv);
> - value *visit_failure_exn;
> + const 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
> 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.21.0
>
> ___
> Libguestfs mailing list
> Libguestfs@redhat.com
> https://www.redhat.com/mailman/listinfo/libguestfs
--
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
___
Libguestfs mailing list
Libguestfs@redhat.com
https://www.redhat.com/mailman/listinfo/libguestfs
[Libguestfs] [PATCH] ocaml: Change calls to caml_named_value() to cope with const value* return.
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), );
- 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
___
Libguestfs mailing list
Libguestfs@redhat.com
https://www.redhat.com/mailman/listinfo/libguestfs
[Libguestfs] [PATCH 1/1] ocaml: make const the return value of caml_named_value()
With OCaml >= 4.09 caml_named_value() returns a const value *, so keep
the constness to build also in this case.
---
common/mlpcre/pcre-c.c | 2 +-
common/mltools/uri-c.c | 2 +-
common/mlvisit/visit-c.c | 2 +-
generator/daemon.ml | 2 +-
4 files changed, 4 insertions(+), 4 deletions(-)
diff --git a/common/mlpcre/pcre-c.c b/common/mlpcre/pcre-c.c
index be054a004..62a6dd5da 100644
--- a/common/mlpcre/pcre-c.c
+++ b/common/mlpcre/pcre-c.c
@@ -73,7 +73,7 @@ init (void)
static void
raise_pcre_error (const char *msg, int errcode)
{
- value *exn = caml_named_value ("PCRE.Error");
+ const value *exn = caml_named_value ("PCRE.Error");
value args[2];
args[0] = caml_copy_string (msg);
diff --git a/common/mltools/uri-c.c b/common/mltools/uri-c.c
index 2a8837cd9..db6fe23fc 100644
--- a/common/mltools/uri-c.c
+++ b/common/mltools/uri-c.c
@@ -47,7 +47,7 @@ guestfs_int_mllib_parse_uri (value argv /* arg value, not an
array! */)
r = parse_uri (String_val (argv), );
if (r == -1) {
-value *exn = caml_named_value ("URI.Parse_failed");
+const value *exn = caml_named_value ("URI.Parse_failed");
caml_raise (*exn);
}
diff --git a/common/mlvisit/visit-c.c b/common/mlvisit/visit-c.c
index 7137c4998..804aabc9d 100644
--- a/common/mlvisit/visit-c.c
+++ b/common/mlvisit/visit-c.c
@@ -53,7 +53,7 @@ value
guestfs_int_mllib_visit (value gv, value dirv, value fv)
{
CAMLparam3 (gv, dirv, fv);
- value *visit_failure_exn;
+ const 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
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.21.0
___
Libguestfs mailing list
Libguestfs@redhat.com
https://www.redhat.com/mailman/listinfo/libguestfs
[Libguestfs] [PATCH 0/1] Build fix for future OCaml 4.09
This is a simple fix for building also with the upcoming OCaml 4.09, which has a slight API change in the C library. This does not cover embedded copies such as ocaml-augeas, and ocaml-libvirt, which are being fixed separately, and will then be synchronized. Pino Toscano (1): ocaml: make const the return value of caml_named_value() common/mlpcre/pcre-c.c | 2 +- common/mltools/uri-c.c | 2 +- common/mlvisit/visit-c.c | 2 +- generator/daemon.ml | 2 +- 4 files changed, 4 insertions(+), 4 deletions(-) -- 2.21.0 ___ Libguestfs mailing list Libguestfs@redhat.com https://www.redhat.com/mailman/listinfo/libguestfs
Re: [Libguestfs] [libnbd PATCH] api: Add way to avoid structured replies
On Wed, Sep 04, 2019 at 01:28:08PM -0500, Eric Blake wrote:
> 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.
There's not really a limit on the length of API names, and in this
case the longer name explains what the option does more clearly.
Anyway this looks fine to me.
ACK
I have one comment unrelated to the patch:
> + "set_request_structured_replies", {
> +default_call with
> +args = [Bool "request"]; ret = RErr;
> +permitted_states = [ Created ];
> +first_version = (1, 2);
I just know that we're going to end up adding new APIs and forgetting
to set the first_version field. There are various things we could do
to prevent this:
(1) In ‘default_call’ set first_version = (0, 0). Update all existing
calls with first_version = (1, 0). Then add a check that
first_version <> (0, 0). There's still a danger of copy and paste
from an existing API, but we should be able to catch that in review.
(2) Store first_version in a separate table. Add checks to ensure the
new table exhaustively covers all APIs. It should be obvious when
submitting a new API that the first_version table must be updated and
what to add here:
let first_version = [
"set_debug", (1, 0);
...
"set_request_structured_replies", (1, 2);
"get_request_structured_replies", (1, 2);
]
Not sure which is better.
Rich.
--
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://www.redhat.com/mailman/listinfo/libguestfs
