June 2019 - Libguestfs - libguestfs List Archives

[nbdkit PATCH 0/2] Reduce network overhead with corking
by Eric Blake 07 Jun '19

07 Jun '19

Slightly RFC, as I need more time to investigate why Unix sockets appeared to degrade with this patch. But as TCP sockets (over loopback to localhost) and TLS sessions (regardless of underlying Unix or TCP) both showed improvements, this looks like a worthwhile series. Eric Blake (2): server: Add support for corking server: Cork around grouped transmission send()s server/internal.h | 3 +++ server/connections.c | 27 +++++++++++++++++++++++ server/crypto.c | 19 ++++++++++++++++ server/protocol.c | 52 ++++++++++++++++++++++++++++++++++++++++++++ 4 files changed, 101 insertions(+) -- 2.20.1

1 4

The way of implementing structs of Rust bindings
by Hiroyuki Katsura 07 Jun '19

07 Jun '19

Hi, I'm now implementing generators of Rust, and I faced a problem. In order for the wrapper to 'understand' the struct passed from C API, it is required to incorporate the definition of the struct in C into Rust code. I have two approaches. 1. Create raw struct(#[repr(C)]), which has the equivalent memory mapping to C struct and access through this struct in Rust 2. Use bindgen to create ffi struct from guestfs.h Each of them has advantages and disadvantages. # 1st approach This is highly dependent on the implementation of API because it is subject to the memory mapping of API struct. When the way struct is structured changes in API, you have to modify Rust bindings. In order to avoid this situation, you can define the same struct in C and Rust and then access the API struct in C and then Rust accesses the data through this struct. This means that ``` API: struct guestfs_A: - field: x FString C: struct RawA { char* x }; struct RawA to_RawA(struct guestfs_A* src) { struct RawA x = {src->x}; return x; } Rust: #repr(C) use std::ffi::CStr; use std::str; #[repr(C)] struct RawA { x: *const i8 } enum guestfs_A {} // opaque struct extern { fn to_RawA( src: *const guestfs_A ) -> RawA; } struct A { x: String } impl A { fn new(src: *const guestfs_A) -> A { let dst = unsafe {to_RawA(src)}; let c_str = unsafe { CStr::from_ptr(dst.x) }; let s = c_str.to_str().unwrap().to_string(); A{ x: s } } } ``` This is a little verbose and inefficient. # 2nd approach The above is easily done by 'bindgen', which automatically generates rust ffi bindings to C library. By using this, API struct is automatically generated. However, it requires a new dependency: libclang. # Question Which of these approaches is more preferable? Regards, Hiroyuki Katsura

3 3

[libnbd PATCH] api: Add nbd_supports_tls
by Eric Blake 05 Jun '19

05 Jun '19

This is slightly redundant with just trying nbd_set_tls(nbd, 2) then checking for failure; however, this function does not set errors and looks more similar to nbd_supports_uri. --- This is borderline enough that I figured I'd post it to check if we want it. generator/generator | 45 ++++++++++++++++++++++++++++++++++++++------- interop/interop.c | 4 ++++ lib/handle.c | 12 ++++++++++++ 3 files changed, 54 insertions(+), 7 deletions(-) diff --git a/generator/generator b/generator/generator index ea6eea4..d21e786 100755 --- a/generator/generator +++ b/generator/generator @@ -971,7 +971,9 @@ the path to the certificates directory (C<nbd_set_tls_certificates>), the username (C<nbd_set_tls_username>) and/or the Pre-Shared Keys (PSK) file (C<nbd_set_tls_psk_file>). For now, when using C<nbd_connect_uri>, any URI query parameters related to -TLS are not handled automatically. +TLS are not handled automatically. Setting the level higher than +zero will fail if libnbd was not compiled against gnutls; you can +test whether this is the case with C<nbd_supports_tls>. For more information see L<libnbd(3)/ENCRYPTION AND AUTHENTICATION>."; }; @@ -995,7 +997,11 @@ set and TLS is used then a compiled in default is used. For root this is C</etc/pki/libnbd/>. For non-root this is C<$HOME/.pki/libnbd> and C<$HOME/.config/pki/libnbd>. If none of these directories can be found then the system -trusted CAs are used."; +trusted CAs are used. + +This function may be called regardless of whether TLS is +supported, but will have no effect unless C<nbd_set_tls> +is also used to request or require TLS."; }; (* Can't implement this because we need a way to return string that @@ -1018,7 +1024,11 @@ Get the current TLS directory. See C<nbd_set_tls_certificates>."; Set this flag to control whether libnbd will verify the identity of the server from the server's certificate and the certificate authority. This defaults to true when connecting to TCP servers -using TLS certificate authentication, and false otherwise."; +using TLS certificate authentication, and false otherwise. + +This function may be called regardless of whether TLS is +supported, but will have no effect unless C<nbd_set_tls> +is also used to request or require TLS."; }; "get_tls_verify_peer", { @@ -1037,7 +1047,11 @@ Get the verify peer flag."; longdesc = "\ Set the TLS client username. This is used if authenticating with PSK over TLS is enabled. -If not set then the local username is used."; +If not set then the local username is used. + +This function may be called regardless of whether TLS is +supported, but will have no effect unless C<nbd_set_tls> +is also used to request or require TLS."; }; "get_tls_username", { @@ -1057,7 +1071,11 @@ Get the current TLS username. See C<nbd_set_tls_username>."; Set the TLS Pre-Shared Keys (PSK) filename. This is used if trying to authenticate to the server using with a pre-shared key. There is no default so if this is not set then PSK -authentication cannot be used to connect to the server."; +authentication cannot be used to connect to the server. + +This function may be called regardless of whether TLS is +supported, but will have no effect unless C<nbd_set_tls> +is also used to request or require TLS."; }; (* Can't implement this because we need a way to return string that @@ -1112,7 +1130,9 @@ C<nbd_connect_tcp> or C<nbd_connect_unix>. This call returns when the connection has been made. This call will fail if libnbd was not compiled with libxml2; you can -test whether this is the case with C<nbd_supports_uri>."; +test whether this is the case with C<nbd_supports_uri>. Support for +URIs that require TLS will fail if libnbd was not compiled with +gnutls; you can test whether this is the case with C<nbd_supports_tls>."; }; "connect_unix", { @@ -1497,7 +1517,9 @@ and completed the NBD handshake by calling C<nbd_aio_is_ready>, on the connection. This call will fail if libnbd was not compiled with libxml2; you can -test whether this is the case with C<nbd_supports_uri>."; +test whether this is the case with C<nbd_supports_uri>. Support for +URIs that require TLS will fail if libnbd was not compiled with +gnutls; you can test whether this is the case with C<nbd_supports_tls>."; }; "aio_connect_unix", { @@ -1876,6 +1898,15 @@ The release number is incremented for each release along a particular branch."; }; + "supports_tls", { + default_call with + args = []; ret = RBool; is_locked = false; may_set_error = false; + shortdesc = "return true if libnbd was compiled with support for TLS"; + longdesc = "\ +Returns true if libnbd was compiled with gnutls which is required +to support TLS encryption, or false if not. See C<nbd_set_tls>."; + }; + "supports_uri", { default_call with args = []; ret = RBool; is_locked = false; may_set_error = false; diff --git a/interop/interop.c b/interop/interop.c index 24f79cc..5d129a0 100644 --- a/interop/interop.c +++ b/interop/interop.c @@ -71,6 +71,10 @@ main (int argc, char *argv[]) /* Require TLS on the handle and fail if not available or if the * handshake fails. */ + if (nbd_supports_tls (nbd) != 1) { + fprintf (stderr, "skip: compiled without TLS supports\n"); + exit (77); + } if (nbd_set_tls (nbd, 2) == -1) { fprintf (stderr, "%s\n", nbd_get_error ()); exit (EXIT_FAILURE); diff --git a/lib/handle.c b/lib/handle.c index cc311ba..e40b274 100644 --- a/lib/handle.c +++ b/lib/handle.c @@ -227,6 +227,18 @@ nbd_unlocked_get_version (struct nbd_handle *h) return PACKAGE_VERSION; } +/* NB: is_locked = false, may_set_error = false. */ +int +nbd_unlocked_supports_tls (struct nbd_handle *h) +{ +#ifdef HAVE_GNUTLS + return 1; +#else + return 0; +#endif +} + +/* NB: is_locked = false, may_set_error = false. */ int nbd_unlocked_supports_uri (struct nbd_handle *h) { -- 2.20.1

2 1

Re: [Libguestfs] [libguestfs/libguestfs] OpenStack support (#38)
by Richard W.M. Jones 05 Jun '19

05 Jun '19

On Wed, Jun 05, 2019 at 04:13:37PM +0000, kedorlaomer wrote: > According to the manpage, > > > Virt-p2v converts a physical machine to run virtualized on KVM, > > managed by libvirt, OpenStack, oVirt, Red Hat Virtualisation > > (RHV), or one of the other targets supported by virt-v2v(1). > > In fact, `openstack` is one of the targets of `virt-v2v`. According > to the file `p2v/conversion.c`, function `generate_wrapper_script`, > the output mode of `virt-v2v` can be set to `glance`. The manpage of > `virt-v2v` claims > > > only -o openstack should be used normally. > > But even if `-o glance` was appropriate, there's no provision for > setting additional command line flags such as `-oo …` in order to > set up the server id, authentication and other conversion options. > > Does the conversion to OpenStack work in any setup? Has it > reportedly ever worked? What needs to be done to get it working? Yes, the -o openstack mode does work and is used routinely. However virt-p2v needs additional work in order to support -o openstack, currently you can only convert to a flat image and then import that. Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com virt-p2v converts physical machines to virtual machines. Boot with a live CD or over the network (PXE) and turn machines into KVM guests. http://libguestfs.org/virt-v2v

1 0

[PATCH libnbd v2] lib: Atomically update h->state when leaving the locked region.
by Richard W.M. Jones 05 Jun '19

05 Jun '19

Split h->state into: - h->public_state = the state on entry to the locked region This is also the atomicly, publicly visible state. - h->state = the real current state of the handle When we leave the locked region we update h->public_state with h->state, so that from outside the lock the handle appears to move atomically from its previous state to the final state without going through any intermediate states. Some calls to ‘get_state’ become calls to ‘get_next_state’ if the need the real state. Others which need to see the publicly visible state are changed to ‘get_public_state’. All calls to ‘set_state’ become ‘set_next_state’ because that is the real state that gets updated. The purpose of this patch is to make it easier to reason about the state in lockless code. --- generator/generator | 29 ++++++++++++++++------------- lib/connect.c | 10 +++++----- lib/disconnect.c | 8 ++++---- lib/handle.c | 2 +- lib/internal.h | 17 ++++++++++++++--- lib/is-state.c | 28 ++++++++++++++++------------ lib/rw.c | 4 ++-- 7 files changed, 58 insertions(+), 40 deletions(-) diff --git a/generator/generator b/generator/generator index 5faa897..246eaed 100755 --- a/generator/generator +++ b/generator/generator @@ -812,8 +812,8 @@ type call = { (* List of permitted states for making this call. [[]] = Any state. *) permitted_states : permitted_state list; (* Most functions must take a lock. The only known exception is - * for a function which {b only} reads from the atomic [h->state] - * field and does nothing else with the handle. + * for a function which {b only} reads from the atomic + * [get_public_state] field and does nothing else with the handle. *) is_locked : bool; (* Most functions can call set_error. For functions which are @@ -2418,11 +2418,11 @@ let generate_lib_states_c () = pr " enum state next_state = %s;\n" state_enum; pr "\n"; pr " r = _enter_%s (h, &next_state, blocked);\n" state_enum; - pr " if (get_state (h) != next_state) {\n"; + pr " if (get_next_state (h) != next_state) {\n"; pr " debug (h, \"transition: %%s -> %%s\",\n"; pr " \"%s\",\n" display_name; pr " nbd_internal_state_short_string (next_state));\n"; - pr " set_state (h, next_state);\n"; + pr " set_next_state (h, next_state);\n"; pr " }\n"; pr " return r;\n"; pr "}\n"; @@ -2437,7 +2437,7 @@ let generate_lib_states_c () = pr " bool blocked;\n"; pr "\n"; pr " /* Validate and handle the external event. */\n"; - pr " switch (get_state (h))\n"; + pr " switch (get_next_state (h))\n"; pr " {\n"; List.iter ( fun ({ parsed = { display_name; state_enum; events } } as state) -> @@ -2449,7 +2449,7 @@ let generate_lib_states_c () = fun (e, next_state) -> pr " case %s:\n" (c_string_of_external_event e); if state != next_state then ( - pr " set_state (h, %s);\n" next_state.parsed.state_enum; + pr " set_next_state (h, %s);\n" next_state.parsed.state_enum; pr " debug (h, \"event %%s: %%s -> %%s\",\n"; pr " \"%s\", \"%s\", \"%s\");\n" (string_of_external_event e) @@ -2465,7 +2465,7 @@ let generate_lib_states_c () = pr " }\n"; pr "\n"; pr " set_error (EINVAL, \"external event %%d is invalid in state %%s\",\n"; - pr " ev, nbd_internal_state_short_string (get_state (h)));\n"; + pr " ev, nbd_internal_state_short_string (get_next_state (h)));\n"; pr " return -1;\n"; pr "\n"; pr " ok:\n"; @@ -2473,7 +2473,7 @@ let generate_lib_states_c () = pr " blocked = true;\n"; pr "\n"; pr " /* Run a single step. */\n"; - pr " switch (get_state (h))\n"; + pr " switch (get_next_state (h))\n"; pr " {\n"; List.iter ( fun { parsed = { state_enum } } -> @@ -2499,7 +2499,7 @@ let generate_lib_states_c () = pr "{\n"; pr " int r = 0;\n"; pr "\n"; - pr " switch (get_state (h))\n"; + pr " switch (get_next_state (h))\n"; pr " {\n"; List.iter ( fun ({ parsed = { state_enum; events } }) -> @@ -2545,7 +2545,7 @@ let generate_lib_states_c () = pr "const char *\n"; pr "nbd_unlocked_connection_state (struct nbd_handle *h)\n"; pr "{\n"; - pr " switch (get_state (h))\n"; + pr " switch (get_next_state (h))\n"; pr " {\n"; List.iter ( fun ({ comment; parsed = { display_name; state_enum } }) -> @@ -2844,7 +2844,7 @@ let generate_lib_api_c () = pr " /* We can check the state outside the handle lock because the\n"; pr " * the state is atomic.\n"; pr " */\n"; - pr " enum state state = get_state (h);\n"; + pr " enum state state = get_public_state (h);\n"; let tests = List.map ( function @@ -2869,8 +2869,11 @@ let generate_lib_api_c () = let argnames = List.flatten (List.map name_of_arg args) in List.iter (pr ", %s") argnames; pr ");\n"; - if is_locked then - pr " pthread_mutex_unlock (&h->lock);\n"; + if is_locked then ( + pr " if (h->public_state != get_next_state (h))\n"; + pr " h->public_state = get_next_state (h);\n"; + pr " pthread_mutex_unlock (&h->lock);\n" + ); pr " return ret;\n"; pr "}\n"; pr "\n"; diff --git a/lib/connect.c b/lib/connect.c index b889f80..4e3141f 100644 --- a/lib/connect.c +++ b/lib/connect.c @@ -38,16 +38,16 @@ static int error_unless_ready (struct nbd_handle *h) { - if (nbd_internal_is_state_ready (get_state (h))) + if (nbd_internal_is_state_ready (get_next_state (h))) return 0; /* Why did it fail? */ - if (nbd_internal_is_state_closed (get_state (h))) { + if (nbd_internal_is_state_closed (get_next_state (h))) { set_error (0, "connection is closed"); return -1; } - if (nbd_internal_is_state_dead (get_state (h))) + if (nbd_internal_is_state_dead (get_next_state (h))) /* Don't set the error here, keep the error set when * the connection died. */ @@ -55,14 +55,14 @@ error_unless_ready (struct nbd_handle *h) /* Should probably never happen. */ set_error (0, "connection in an unexpected state (%s)", - nbd_internal_state_short_string (get_state (h))); + nbd_internal_state_short_string (get_next_state (h))); return -1; } static int wait_until_connected (struct nbd_handle *h) { - while (nbd_internal_is_state_connecting (get_state (h))) { + while (nbd_internal_is_state_connecting (get_next_state (h))) { if (nbd_unlocked_poll (h, -1) == -1) return -1; } diff --git a/lib/disconnect.c b/lib/disconnect.c index 423edaf..95e9a37 100644 --- a/lib/disconnect.c +++ b/lib/disconnect.c @@ -29,14 +29,14 @@ int nbd_unlocked_shutdown (struct nbd_handle *h) { - if (nbd_internal_is_state_ready (get_state (h)) || - nbd_internal_is_state_processing (get_state (h))) { + if (nbd_internal_is_state_ready (get_next_state (h)) || + nbd_internal_is_state_processing (get_next_state (h))) { if (nbd_unlocked_aio_disconnect (h, 0) == -1) return -1; } - while (!nbd_internal_is_state_closed (get_state (h)) && - !nbd_internal_is_state_dead (get_state (h))) { + while (!nbd_internal_is_state_closed (get_next_state (h)) && + !nbd_internal_is_state_dead (get_next_state (h))) { if (nbd_unlocked_poll (h, -1) == -1) return -1; } diff --git a/lib/handle.c b/lib/handle.c index cc311ba..4167b3e 100644 --- a/lib/handle.c +++ b/lib/handle.c @@ -57,7 +57,7 @@ nbd_create (void) s = getenv ("LIBNBD_DEBUG"); h->debug = s && strcmp (s, "1") == 0; - h->state = STATE_START; + h->state = h->public_state = STATE_START; h->pid = -1; h->export_name = strdup (""); diff --git a/lib/internal.h b/lib/internal.h index 5b6152b..ce4bf5b 100644 --- a/lib/internal.h +++ b/lib/internal.h @@ -80,7 +80,17 @@ struct nbd_handle { /* Linked list of close callbacks. */ struct close_callback *close_callbacks; - _Atomic enum state state; /* State machine. */ + /* State machine. + * + * The actual current state is ‘state’. ‘public_state’ is updated + * before we release the lock. + * + * Note don't access these fields directly, use the SET_NEXT_STATE + * macro in generator/states* code, or the set_next_state, + * get_next_state and get_public_state macros in regular code. + */ + _Atomic enum state public_state; + enum state state; bool structured_replies; /* If we negotiated NBD_OPT_STRUCTURED_REPLY */ @@ -291,8 +301,9 @@ extern const char *nbd_internal_state_short_string (enum state state); extern enum state_group nbd_internal_state_group (enum state state); extern enum state_group nbd_internal_state_group_parent (enum state_group group); -#define set_state(h,next_state) ((h)->state) = (next_state) -#define get_state(h) ((h)->state) +#define set_next_state(h,next_state) ((h)->state) = (next_state) +#define get_next_state(h) ((h)->state) +#define get_public_state(h) ((h)->public_state) /* utils.c */ extern void nbd_internal_hexdump (const void *data, size_t len, FILE *fp); diff --git a/lib/is-state.c b/lib/is-state.c index c941ab4..b2c20df 100644 --- a/lib/is-state.c +++ b/lib/is-state.c @@ -98,44 +98,48 @@ nbd_internal_is_state_closed (enum state state) return state == STATE_CLOSED; } -/* NB: is_locked = false, may_set_error = false. */ +/* The nbd_unlocked_aio_is_* calls are the public APIs + * for reading the state of the handle. + * + * They all have: is_locked = false, may_set_error = false. + * + * They all read the public state, not the real state. Therefore you + * SHOULD NOT call these functions from elsewhere in the library (use + * nbd_internal_is_* instead). + */ + int nbd_unlocked_aio_is_created (struct nbd_handle *h) { - return nbd_internal_is_state_created (get_state (h)); + return nbd_internal_is_state_created (get_public_state (h)); } -/* NB: is_locked = false, may_set_error = false. */ int nbd_unlocked_aio_is_connecting (struct nbd_handle *h) { - return nbd_internal_is_state_connecting (get_state (h)); + return nbd_internal_is_state_connecting (get_public_state (h)); } -/* NB: is_locked = false, may_set_error = false. */ int nbd_unlocked_aio_is_ready (struct nbd_handle *h) { - return nbd_internal_is_state_ready (get_state (h)); + return nbd_internal_is_state_ready (get_public_state (h)); } -/* NB: is_locked = false, may_set_error = false. */ int nbd_unlocked_aio_is_processing (struct nbd_handle *h) { - return nbd_internal_is_state_processing (get_state (h)); + return nbd_internal_is_state_processing (get_public_state (h)); } -/* NB: is_locked = false, may_set_error = false. */ int nbd_unlocked_aio_is_dead (struct nbd_handle *h) { - return nbd_internal_is_state_dead (get_state (h)); + return nbd_internal_is_state_dead (get_public_state (h)); } -/* NB: is_locked = false, may_set_error = false. */ int nbd_unlocked_aio_is_closed (struct nbd_handle *h) { - return nbd_internal_is_state_closed (get_state (h)); + return nbd_internal_is_state_closed (get_public_state (h)); } diff --git a/lib/rw.c b/lib/rw.c index b38d95b..ad9c8a0 100644 --- a/lib/rw.c +++ b/lib/rw.c @@ -201,7 +201,7 @@ nbd_internal_command_common (struct nbd_handle *h, * be handled automatically on a future cycle around to READY. */ if (h->cmds_to_issue != NULL) { - assert (nbd_internal_is_state_processing (get_state (h))); + assert (nbd_internal_is_state_processing (get_next_state (h))); prev_cmd = h->cmds_to_issue; while (prev_cmd->next) prev_cmd = prev_cmd->next; @@ -209,7 +209,7 @@ nbd_internal_command_common (struct nbd_handle *h, } else { h->cmds_to_issue = cmd; - if (nbd_internal_is_state_ready (get_state (h)) && + if (nbd_internal_is_state_ready (get_next_state (h)) && nbd_internal_run (h, cmd_issue) == -1) return -1; } -- 2.21.0

2 1

[libnbd PATCH 0/2] Better handling of failed block_status callback
by Eric Blake 05 Jun '19

05 Jun '19

Rather than moving the connection to DEAD, we can just ignore further contexts to the existing command handle, and fail the overall command with the same errno as the failed callback. Eric Blake (2): states: Track cmd->error as errno, not wire value api: Recover from block status callback failure generator/generator | 5 ++- generator/states-reply-simple.c | 2 +- generator/states-reply-structured.c | 61 ++++++++++++++------------- interop/dirty-bitmap.c | 65 +++++++++++++++++++++++------ lib/aio.c | 1 - lib/internal.h | 2 +- 6 files changed, 91 insertions(+), 45 deletions(-) -- 2.20.1

2 3

[PATCH libnbd] generator: Fix race condition when validating h->state.
by Richard W.M. Jones 04 Jun '19

04 Jun '19

The code for (eg) nbd_aio_pread starts by checking this outside the lock: if (!(nbd_aio_is_ready (h) || nbd_aio_is_processing (h))) { However there can still be a race condition even though h->state is atomic: Thread A Thread B (in a call that holds h->lock) (calling nbd_aio_pread) -------------------------------------------------------------------- h->state is processing checks nbd_aio_is_ready (it's false) h->state is moved to READY checks nbd_aio_is_processing (it's false) validation check fails However the state was valid so the validation check should have succeeded. Fixes commit e63a11736930c381a79a8cc2d03844cfff5db3ef. Thanks: Eric Blake for identifying the race condition above. --- generator/generator | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/generator/generator b/generator/generator index ea6f739..8068762 100755 --- a/generator/generator +++ b/generator/generator @@ -2820,10 +2820,9 @@ let generate_lib_api_c () = pr " nbd_internal_reset_error (\"nbd_%s\");\n" name; pr "\n" ); + if is_locked then + pr " pthread_mutex_lock (&h->lock);\n"; if permitted_states <> [] then ( - pr " /* We can check the state outside the handle lock because the\n"; - pr " * the state is atomic.\n"; - pr " */\n"; let tests = List.map ( function @@ -2842,8 +2841,6 @@ let generate_lib_api_c () = pr " }\n"; pr "\n" ); - if is_locked then - pr " pthread_mutex_lock (&h->lock);\n"; pr " ret = nbd_unlocked_%s (h" name; let argnames = List.flatten (List.map name_of_arg args) in List.iter (pr ", %s") argnames; -- 2.21.0

2 2

[libnbd PATCH] generator: Add #define witnesses for all API
by Eric Blake 04 Jun '19

04 Jun '19

Make it easier for C libraries to consume arbitrary versions of libnbd, by giving a probe for which functions the current version of the library exports. --- I'm fuzzy enough on OCaml that I'll get review for this, although I like the resulting libnbd.h. generator/generator | 24 +++++++++++++++++++++++- 1 file changed, 23 insertions(+), 1 deletion(-) diff --git a/generator/generator b/generator/generator index db7c10f..7d0ea3f 100755 --- a/generator/generator +++ b/generator/generator @@ -2712,6 +2712,12 @@ let print_extern name args ret = print_call name args ret; pr ";\n" +let print_extern_and_define name args ret = + let name_upper = String.uppercase_ascii name in + print_extern name args ret; + pr "#define LIBNBD_HAVE_NBD_%s 1\n" name_upper; + pr "\n" + let generate_include_libnbd_h () = generate_header CStyle; @@ -2729,14 +2735,23 @@ let generate_include_libnbd_h () = List.iter (fun (n, i) -> pr "#define LIBNBD_%-30s %d\n" n i) constants; pr "\n"; pr "extern struct nbd_handle *nbd_create (void);\n"; + pr "#define LIBNBD_HAVE_NBD_CREATE 1\n"; + pr "\n"; pr "extern void nbd_close (struct nbd_handle *h);\n"; + pr "#define LIBNBD_HAVE_NBD_CLOSE 1\n"; + pr "\n"; pr "extern const char *nbd_get_error (void);\n"; + pr "#define LIBNBD_HAVE_NBD_GET_ERROR 1\n"; + pr "\n"; pr "extern int nbd_get_errno (void);\n"; + pr "#define LIBNBD_HAVE_NBD_GET_ERRNO 1\n"; + pr "\n"; pr "extern int nbd_add_close_callback (struct nbd_handle *h,\n"; pr " nbd_close_callback cb, void *data);\n"; + pr "#define LIBNBD_HAVE_NBD_ADD_CLOSE_CALLBACK 1\n"; pr "\n"; List.iter ( - fun (name, { args; ret }) -> print_extern name args ret + fun (name, { args; ret }) -> print_extern_and_define name args ret ) handle_calls; pr "\n"; pr "#endif /* LIBNBD_H */\n" @@ -2933,6 +2948,13 @@ in detail. If you want an overview of using the API, or to see how to call the API from other programming languages, start with libnbd(3). +For the sake of conditional compilation across a range of libnbd +versions, where a client may take advantage of newer API when present +but gracefully continue to compile even when it is not, all functions +declared in B<E<lt>libnbd.hE<gt>> have a corresponding witness macro +with prefix C<LIBNBD_HAVE_>. For example, C<nbd_create> has a +counterpart macro C<LIBNBD_HAVE_NBD_CREATE> defined to C<1>. + =head1 CREATE, GET AND CLOSE HANDLES struct nbd_handle *nbd; -- 2.20.1

2 1

[PATCH libnbd discussion only 0/5] api: Implement concurrent writer.
by Richard W.M. Jones 03 Jun '19

03 Jun '19

This works, but there's no time saving and I'm still investigating whether it does what I think it does. Nevertheless I thought I would post it because it (probably) implements the idea I had last night outlined in: https://www.redhat.com/archives/libguestfs/2019-June/msg00010.html The meat of the change is patch 4. Patch 5 is an example which I would probably fold into patch 4 for submission. The others are non-controversial documentation and refactorings to prepare for the change. Rich.

2 12

[PATCH libnbd] api: nbd_get_version, nbd_supports_uri and nbd_get_package_name.
by Richard W.M. Jones 03 Jun '19

03 Jun '19

nbd_get_version returns the library version as a string. nbd_supports_uri returns whether or not the library was compiled with NBD URI support (ie. with libxml2). nbd_get_package_name is fairly useless as it always returns the string "libnbd", however it replaces a function that was written for the Python bindings. These take a handle parameter but don't need to use it. Changing the generator to support a whole new class of API calls which don't need a handle is a massive pain. --- generator/generator | 51 +++++++++++++++++++++++++++++++++++++++++---- lib/handle.c | 22 +++++++++++++++++++ python/handle.c | 22 ------------------- 3 files changed, 69 insertions(+), 26 deletions(-) diff --git a/generator/generator b/generator/generator index 54dc6cc..5e052df 100755 --- a/generator/generator +++ b/generator/generator @@ -1823,6 +1823,49 @@ can be used for debugging or troubleshooting, but you should not rely on the state of connections since it may change in future versions."; }; + + "get_package_name", { + default_call with + args = []; ret = RConstString; is_locked = false; may_set_error = false; + shortdesc = "return the name of the library"; + longdesc = "\ +Returns the name of the library, always C<\"libnbd\"> unless +the library was modified with another name at compile time."; + }; + + "get_version", { + default_call with + args = []; ret = RConstString; is_locked = false; may_set_error = false; + shortdesc = "return a descriptive string for the state of the connection"; + longdesc = "\ +Return the version of libnbd. This is returned as a string +in the form C<\"major.minor.release\"> where each of major, minor +and release is a small positive integer. For example C<\"1.0.3\">. + +The major number is C<0> for the early experimental versions of +libnbd where we still had an unstable API, or C<1> for the versions +of libnbd with a long-term stable API and ABI. + +The minor number is even (C<0>, C<2>, etc) for stable releases, +and odd (C<1>, C<3>, etc) for development versions. Note that +new APIs added in a development version remain experimental +and subject to change in that branch until they appear in a stable +release. + +The release number is increments for each release along a particular +branch."; + }; + + "supports_uri", { + default_call with + args = []; ret = RBool; is_locked = false; may_set_error = false; + shortdesc = "return true if libnbd was compiled with support for NBD URIs"; + longdesc = "\ +Returns true if libnbd was compiled with libxml2 which is required +to support NBD URIs, or false if not. See C<nbd_connect_uri> and +C<nbd_aio_connect_uri>."; + }; + ] (* Constants, flags, etc. *) @@ -3007,7 +3050,7 @@ get_handle (PyObject *obj) fun name -> pr "extern PyObject *nbd_internal_py_%s (PyObject *self, PyObject *args);\n" name; - ) ([ "create"; "close"; "get_package_name"; "get_package_version"; + ) ([ "create"; "close"; "alloc_aio_buffer"; "aio_buffer_from_bytearray"; "aio_buffer_to_bytearray" ] @ List.map fst handle_calls); @@ -3035,7 +3078,7 @@ let generate_python_libnbdmod_c () = fun name -> pr " { (char *) \"%s\", nbd_internal_py_%s, METH_VARARGS, NULL },\n" name name; - ) ([ "create"; "close"; "get_package_name"; "get_package_version"; + ) ([ "create"; "close"; "alloc_aio_buffer"; "aio_buffer_from_bytearray"; "aio_buffer_to_bytearray" ] @ List.map fst handle_calls); pr " { NULL, NULL, 0, NULL }\n"; @@ -3607,8 +3650,8 @@ class NBD (object): (* For nbdsh. *) pr "\ -package_name = libnbdmod.get_package_name () -__version__ = libnbdmod.get_package_version () +package_name = NBD().get_package_name() +__version__ = NBD().get_version() if __name__ == \"__main__\": import argparse diff --git a/lib/handle.c b/lib/handle.c index a42b8dc..cc311ba 100644 --- a/lib/handle.c +++ b/lib/handle.c @@ -214,3 +214,25 @@ nbd_add_close_callback (struct nbd_handle *h, nbd_close_callback cb, void *data) pthread_mutex_unlock (&h->lock); return ret; } + +const char * +nbd_unlocked_get_package_name (struct nbd_handle *h) +{ + return PACKAGE_NAME; +} + +const char * +nbd_unlocked_get_version (struct nbd_handle *h) +{ + return PACKAGE_VERSION; +} + +int +nbd_unlocked_supports_uri (struct nbd_handle *h) +{ +#ifdef HAVE_LIBXML2 + return 1; +#else + return 0; +#endif +} diff --git a/python/handle.c b/python/handle.c index 7b5e139..7cff41a 100644 --- a/python/handle.c +++ b/python/handle.c @@ -89,28 +89,6 @@ free_aio_buffer (PyObject *capsule) free (buf); } -PyObject * -nbd_internal_py_get_package_name (PyObject *self, PyObject *args) -{ - static char name[] = PACKAGE_NAME; - - if (!PyArg_ParseTuple (args, (char *) ":nbd_internal_py_get_package_name")) - return NULL; - - return PyUnicode_FromStringAndSize (name, strlen (name)); -} - -PyObject * -nbd_internal_py_get_package_version (PyObject *self, PyObject *args) -{ - static char version[] = PACKAGE_VERSION; - - if (!PyArg_ParseTuple (args, (char *) ":nbd_internal_py_get_package_version")) - return NULL; - - return PyUnicode_FromStringAndSize (version, strlen (version)); -} - /* Allocate a persistent buffer used for nbd_aio_pread and * nbd_aio_pwrite. */ -- 2.21.0

2 3