Re: [Libguestfs] [libnbd PATCH 07/10] rust: Add a couple of integration tests
On Wed, Jul 19, 2023 at 09:09:51AM +, Tage Johansson wrote:
> A couple of integration tests are added in rust/tests. They are mostly
> ported from the OCaml tests.
> ---
> rust/tests/test_210_opt_abort.rs | 39 +
> rust/tests/test_220_opt_list.rs | 85 +++
I noticed something today while working on issues with nbd_opt_abort
vs. nbd_shutdown:
> diff --git a/rust/tests/test_220_opt_list.rs b/rust/tests/test_220_opt_list.rs
> new file mode 100644
> index 000..5abec5f
> --- /dev/null
> +++ b/rust/tests/test_220_opt_list.rs
> @@ -0,0 +1,85 @@
> +
> +impl ConnTester {
> +fn new() -> Self {
> +let srcdir = env::var("srcdir").unwrap();
> +let srcdir = Path::new();
> +let script_path = srcdir.join("../tests/opt-list.sh");
> +let script_path =
> +CString::new(script_path.into_os_string().into_vec()).unwrap();
> +Self { script_path }
> +}
> +
> +fn connect(
This function is modeled after the 'let conn' function in
test_220_opt_list.ml; however,...
> +,
> +mode: u8,
> +expected_exports: &[],
> +) -> libnbd::Result<()> {
> +let nbd = libnbd::Handle::new().unwrap();
> +nbd.set_opt_mode(true).unwrap();
> +nbd.connect_command(&[
> +c_str!("nbdkit"),
> +c_str!("-s"),
> +c_str!("--exit-with-parent"),
> +c_str!("-v"),
> +c_str!("sh"),
> +_path,
> +::new(format!("mode={mode}")).unwrap(),
> +])
> +.unwrap();
> +
> +// Collect all exports in this list.
> +let exports = Arc::new(Mutex::new(Vec::new()));
> +let exports_clone = exports.clone();
> +let count = nbd.opt_list(move |name, _| {
> +exports_clone.lock().unwrap().push(name.to_owned());
> +0
> +})?;
> +let exports =
> Arc::into_inner(exports).unwrap().into_inner().unwrap();
> +assert_eq!(exports.len(), count as usize);
> +assert_eq!(exports.len(), expected_exports.len());
> +for (export, ) in exports.iter().zip(expected_exports) {
> +assert_eq!(export.as_c_str(), expected);
> +}
> +Ok(())
...the OCaml version calls 'NBD.opt_abort nbd' after verifying that
exports match the expected list, while the Rust code does not. You
can see the same thing in the Python tests (calling opt_abort before
closing the handle). This means that for the Rust tests, the server
is more likely to issue an error message about the client abruptly
disconnecting, which doesn't really affect the test passing or
failing, but adds spurious noise to the log files if we are ever
trying to decipher whether two language bindings are doing the same
thing.
Also affected: 240, 245, and their async counterpart tests.
--
Eric Blake, Principal Software Engineer
Red Hat, Inc.
Virtualization: qemu.org | libguestfs.org
___
Libguestfs mailing list
Libguestfs@redhat.com
https://listman.redhat.com/mailman/listinfo/libguestfs
Re: [Libguestfs] [libnbd PATCH 07/10] rust: Add a couple of integration tests
On 7/19/2023 4:50 PM, Richard W.M. Jones wrote:
On Wed, Jul 19, 2023 at 09:09:51AM +, Tage Johansson wrote:
+fn test_connect_command() {
+let nbd = libnbd::Handle::new().unwrap();
+nbd.connect_command(&[
+c_str!("nbdkit"),
+c_str!("-s"),
+c_str!("--exit-with-parent"),
+c_str!("-v"),
+c_str!("null"),
+])
+.unwrap();
So this doesn't seem very natural Rust to me. For example standard
library exec::execvp lets you just use:
let err = exec::execvp("echo", &["echo", "foo"]);
println!("Error: {}", err);
(https://docs.rs/exec/latest/exec/fn.execvp.html)
Is there a way to get rid of the c_str macro here?
Yes, there is, but it would require cloning every string. The problem is
that Libnbd uses null terminated strings and Rust uses "Vector like"
strings with a stored length (as C++'s `String` type). So a rust string
literal (`str`) like `"nbdkit"` is stored as an array of length 6 bytes
without a NULL at the end, and the integer `6` to tell the length. (See
here for more
explonations.) The thing is that to convert an `` to a null
terminated string (``) it must be cloned to make room for the extra
NULL character.
One would need to use something like `impl Into>` instead of
`` as type of the string arguments, and then convert it to a null
terminated string with `CString::new()`
.
Since these conversions would only take place when the user creates or
configures the handle, the extra cost shouldn't be a problem in most
cases, and I can definitely change this. But it doesn't become zero cost
abstraction.
Best regards,
Tage
So to summerize: I can change the type of strings that are sent to
Libnbd so that the `c_str!(...)` macro wouldn't be needed, although that
would require cloning every string. But I'd rather keep the type of
strings going
Rich.
___
Libguestfs mailing list
Libguestfs@redhat.com
https://listman.redhat.com/mailman/listinfo/libguestfs
Re: [Libguestfs] [libnbd PATCH 07/10] rust: Add a couple of integration tests
On Wed, Jul 19, 2023 at 09:09:51AM +, Tage Johansson wrote:
> +fn test_connect_command() {
> +let nbd = libnbd::Handle::new().unwrap();
> +nbd.connect_command(&[
> +c_str!("nbdkit"),
> +c_str!("-s"),
> +c_str!("--exit-with-parent"),
> +c_str!("-v"),
> +c_str!("null"),
> +])
> +.unwrap();
So this doesn't seem very natural Rust to me. For example standard
library exec::execvp lets you just use:
let err = exec::execvp("echo", &["echo", "foo"]);
println!("Error: {}", err);
(https://docs.rs/exec/latest/exec/fn.execvp.html)
Is there a way to get rid of the c_str macro here?
Rich.
--
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
Fedora Windows cross-compiler. Compile Windows programs, test, and
build Windows installers. Over 100 libraries supported.
http://fedoraproject.org/wiki/MinGW
___
Libguestfs mailing list
Libguestfs@redhat.com
https://listman.redhat.com/mailman/listinfo/libguestfs
