Libguestfs - Jul 2019 - Re: [libnbd PATCH] RFC: Add bindings for Rust language

This is just a basic ugly support, not meant to be pushed (at least for now).
There is quite a lot missing, but there is an example which shows that it really
works.  And valgrind reports that all allocations were freed.

The way the code is generated is also not nice, I wish there was more code
actually written in some files and not generated by the generator (as much
hard-coded static strings as possible), maybe similarly to the states.c, I
don't
know.

It follows the convention of `-sys` packages:

  https://doc.rust-lang.org/cargo/reference/build-scripts.html#a-sys-packages

But that's about it, I hadn't really had much more time to do anything,
I had
only limited free time during the weekend to push this as far as possible and I
got stuck couple of times.  But I think the hardest parts are solved and

Also up for discussion is whether the libnbd crate should be separate since the
higher-level functionality it should provide will not be tightly coupled with
libnbd itself and the releases (especially the numbers) do not need to happen in
sync.

Maybe even the libnbd-sys crate should be in separate repo which has libnbd as a
submodule (in order to use the valuable structuring of API data that is not
installed in the system).

Signed-off-by: Martin Kletzander <mkletzan@redhat.com>
---
 .gitignore                        |   5 +
 Makefile.am                       |   1 +
 configure.ac                      |  16 ++
 generator/generator               | 301 ++++++++++++++++++++++++++++++
 run.in                            |   9 +
 rust/Cargo.toml                   |   6 +
 rust/Makefile.am                  |  42 +++++
 rust/libnbd-sys/Cargo.toml.in     |  10 +
 rust/libnbd-sys/build.rs          |   9 +
 rust/libnbd-sys/src/.gitkeep      |   0
 rust/libnbd/Cargo.toml.in         |   9 +
 rust/libnbd/examples/hello-nbd.rs |   7 +
 rust/libnbd/src/lib.rs            |   4 +
 rust/libnbd/src/nbd_error.rs      |  31 +++
 rust/run-tests                    |   4 +
 15 files changed, 454 insertions(+)
 create mode 100644 rust/Cargo.toml
 create mode 100644 rust/Makefile.am
 create mode 100644 rust/libnbd-sys/Cargo.toml.in
 create mode 100644 rust/libnbd-sys/build.rs
 create mode 100644 rust/libnbd-sys/src/.gitkeep
 create mode 100644 rust/libnbd/Cargo.toml.in
 create mode 100644 rust/libnbd/examples/hello-nbd.rs
 create mode 100644 rust/libnbd/src/lib.rs
 create mode 100644 rust/libnbd/src/nbd_error.rs
 create mode 100755 rust/run-tests

diff --git a/.gitignore b/.gitignore
index d4828fae028b..58ed9d04df09 100644
--- a/.gitignore
+++ b/.gitignore
@@ -86,6 +86,11 @@ Makefile.in
 /python/nbd.py
 /python/run-python-tests
 /run
+/rust/**/Cargo.lock
+/rust/*/Cargo.toml
+/rust/libnbd/src/glue.rs
+/rust/libnbd-sys/src/lib.rs
+/rust/target
 /sh/nbdsh
 /sh/nbdsh.1
 /stamp-h1
diff --git a/Makefile.am b/Makefile.am
index 6a0f4bdfc653..f28f828f6b7d 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -38,6 +38,7 @@ SUBDIRS = \
 	ocaml \
 	ocaml/examples \
 	interop \
+	rust \
 	$(NULL)
 
 noinst_SCRIPTS = run
diff --git a/configure.ac b/configure.ac
index 91d08341d732..339073cf6660 100644
--- a/configure.ac
+++ b/configure.ac
@@ -258,6 +258,19 @@ AS_IF([test "x$enable_python" !=
"xno"],[
 AM_CONDITIONAL([HAVE_PYTHON],
     [test "x$PYTHON" != "xno" && test
"x$have_python_module" = "x1" ])
 
+dnl Rust, optional, just runs tests
+AC_ARG_ENABLE([rust],
+    AS_HELP_STRING([--disable-rust], [disable Rust language binding tests]),
+    [],
+    [enable_rust=yes])
+AS_IF([test "x$enable_rust" != "xno"],
+      [AC_CHECK_PROG([CARGO],[cargo],[cargo],[no])])
+AS_IF([test "$CARGO" = "no"],
+      [AC_MSG_ERROR([cargo not found])])
+
+AM_CONDITIONAL([HAVE_RUST],
+    [test "$CARGO" != "no" && test
"$enable_rust" = "yes" ])
+
 dnl Produce output files.
 AC_CONFIG_HEADERS([config.h])
 
@@ -282,6 +295,9 @@ AC_CONFIG_FILES([Makefile
                  ocaml/META
                  ocaml/examples/Makefile
                  python/Makefile
+                 rust/Makefile
+                 rust/libnbd-sys/Cargo.toml
+                 rust/libnbd/Cargo.toml
                  sh/Makefile
                  tests/Makefile
                  tests/functions.sh
diff --git a/generator/generator b/generator/generator
index 9192988af4b3..969d0af0c247 100755
--- a/generator/generator
+++ b/generator/generator
@@ -2841,6 +2841,9 @@ let ()      | name, { permitted_states = (_::_);
may_set_error = false } ->
        failwithf "%s: if may_set_error is false, permitted_states must be
empty (any permitted state)"
                  name
+    | name, { ret = RErr; may_set_error = false } ->
+       failwithf "%s: if ret is RErr, then may_set_error cannot be
false"
+                 name
     | _ -> ()
   ) handle_calls
 
@@ -4652,6 +4655,302 @@ let generate_ocaml_nbd_c ()  
   List.iter print_ocaml_binding handle_calls
 
+let rec pr_indent = function
+  | 0 -> ()
+  | n -> pr "    "; pr_indent (n - 1)
+
+let rec print_rust_ffi_arg_list ?(handle = false) indent args +  let pri () =
pr_indent indent in
+  let pri2 () = pr_indent (indent + 1) in
+  pr "(\n";
+  if handle then (
+    pri (); pr "h: *mut nbd_handle,\n";
+  );
+  List.iter (
+    fun arg ->
+      (match arg with
+       | ArrayAndLen (UInt32 n, len) ->
+          pri (); pr "%s: *mut u32,\n" n;
+          pri (); pr "%s: usize,\n" len
+       | ArrayAndLen _ -> assert false
+       | Bool n -> pri (); pr "%s: bool,\n" n
+       | BytesIn (n, len)
+       | BytesPersistIn (n, len) ->
+          pri (); pr "%s: *const c_void,\n" n;
+          pri (); pr "%s: usize,\n" len
+       | BytesOut (n, len)
+       | BytesPersistOut (n, len) ->
+          pri (); pr "%s: *mut c_void,\n" n;
+          pri (); pr "%s: usize,\n" len
+       | Callback (n, args)
+       | CallbackPersist (n, args) ->
+         pri (); pr "%s: Option<\n" n;
+         pri2 (); pr "unsafe extern \"C\" fn";
+         print_rust_ffi_arg_list (indent + 2) args;
+         pr " -> c_int,\n";
+         pri (); pr ">,\n"
+       | Flags n -> pri (); pr "%s: u32,\n" n
+       | Int n -> pri (); pr "%s: c_int,\n" n
+       | Int64 n -> pri (); pr "%s: i64,\n" n
+       | Mutable (Int n) -> pri (); pr "%s: *mut c_int,\n" n
+       | Mutable arg -> assert false
+       | Opaque n -> pri (); pr "%s: *mut c_void,\n" n
+       | Path n
+       | String n -> pri (); pr "%s: *const c_char,\n" n
+       | StringList n -> pri (); pr "%s: *mut *mut c_char,\n" n
+       | SockAddrAndLen (n, len) ->
+         pri (); pr "%s: *const libc::sockaddr,\n" n;
+         pri (); pr "%s: libc::socklen_t,\n" len
+       | UInt n -> pri (); pr "%s: c_uint,\n" n
+       | UInt32 n -> pri (); pr "%s: u32,\n" n
+       | UInt64 n -> pri (); pr "%s: u64,\n" n
+      );
+  ) args;
+  pr_indent (indent - 1); pr ")"
+
+let print_rust_arg_list ?(handle = false) indent args +  let pri () = pr_indent
indent in
+  let pri2 () = pr_indent (indent + 1) in
+  pr "(\n";
+  if handle then (
+    pri (); pr "&self,\n";
+  );
+  List.iter (
+    fun arg ->
+      (* TODO: rewrite to use Rust types *)
+      (match arg with
+       | ArrayAndLen (UInt32 n, len) ->
+          pr "%s: *mut u32,\n" n; pri ();
+          pr "%s: usize,\n" len; pri ()
+       | ArrayAndLen _ -> assert false
+       | Bool n -> pri (); pr "%s: bool,\n" n
+       | BytesIn (n, len)
+       | BytesPersistIn (n, len) ->
+          pri (); pr "%s: *const c_void,\n" n;
+          pri (); pr "%s: usize,\n" len
+       | BytesOut (n, len)
+       | BytesPersistOut (n, len) ->
+          pri (); pr "%s: *mut c_void,\n" n;
+          pri (); pr "%s: usize,\n" len
+       | Callback (n, args)
+       | CallbackPersist (n, args) ->
+         pri (); pr "%s: Option<\n" n;
+         pri2 (); pr "unsafe extern \"C\" fn";
+         print_rust_ffi_arg_list (indent + 2) args;
+         pr " -> c_int,\n";
+         pri (); pr ">,\n"
+       | Flags n -> pri (); pr "%s: u32,\n" n
+       | Int n -> pri (); pr "%s: c_int,\n" n
+       | Int64 n -> pri (); pr "%s: i64,\n" n
+       | Mutable (Int n) -> pri (); pr "%s: *mut c_int,\n" n
+       | Mutable arg -> assert false
+       | Opaque n -> pri (); pr "%s: *mut c_void,\n" n
+       | Path n
+       | String n -> pri (); pr "%s: *const c_char,\n" n
+       | StringList n -> pri (); pr "%s: *mut *mut c_char,\n" n
+       | SockAddrAndLen (n, len) ->
+         pri (); pr "%s: *const libc::sockaddr,\n" n;
+         pri (); pr "%s: libc::socklen_t" len
+       | UInt n -> pri (); pr "%s: c_uint,\n" n
+       | UInt32 n -> pri (); pr "%s: u32,\n" n
+       | UInt64 n -> pri (); pr "%s: u64,\n" n
+      );
+  ) args;
+  pr_indent (indent - 1); pr ")"
+
+let generate_rust_sys_lib_rs () +  let print_extern (name, {args; ret; _ }) +  
let ret_rs_type +      match ret with
+      | RBool
+      | RErr
+      | RFd
+      | RInt -> "c_int"
+      | RConstString -> "*const c_char"
+      | RInt64 -> "i64"
+      | RString -> "*mut c_char"
+    in
+
+    (* TODO: print shortdesc and longdesc *)
+    pr "    pub fn nbd_%s" name;
+    print_rust_ffi_arg_list ~handle:true 2 args;
+    pr " -> %s;\n" ret_rs_type
+  in
+
+  generate_header CStyle;
+
+  (* TODO: global documentation *)
+
+  pr "#[allow(unused_imports)]\n";
+  pr "use std::os::raw::{c_char, c_int, c_uint, c_void};\n";
+  pr "\n";
+
+  (* TODO: print constants *)
+  (* TODO: print constants for metadata namespaces *)
+
+  pr "#[link(name = \"nbd\")]";
+  pr "extern \"C\" {\n";
+  pr "    pub fn nbd_create() -> *mut nbd_handle;\n";
+  pr "    pub fn nbd_close(handle: *mut nbd_handle);\n";
+  pr "    pub fn nbd_add_close_callback(\n";
+  pr "        handle: *mut nbd_handle,\n";
+  pr "        callback: Option<unsafe extern \"C\" fn(*mut
c_void)>,\n";
+  pr "        ) -> c_int;\n";
+  pr "";
+  pr "    pub fn nbd_get_error() -> *const c_char;\n";
+  pr "    pub fn nbd_get_errno() -> c_int;\n";
+  pr "\n";
+  List.iter print_extern handle_calls;
+  pr "}\n";
+  pr "\n";
+  pr "#[repr(C)]\n";
+  pr "#[derive(Debug, Copy, Clone)]\n";
+  pr "pub struct nbd_handle {\n";
+  pr "    _unused: [u8; 0],\n";
+  pr "}\n"
+
+let generate_rust_glue_rs () +  let print_wrapper (name, {args; ret;
permitted_states;
+                            is_locked; may_set_error}) +    let ret_rs_type +  
let typ +        match ret with
+        | RBool -> "bool"
+        | RErr -> "()"
+        | RFd -> "RawFd"
+        | RInt -> "i32"
+        | RConstString -> "&'static str"
+        | RInt64 -> "i64"
+        | RString -> "String" in
+      if may_set_error then "Result<" ^ typ ^ ",
NbdError>" else typ
+    in
+
+    (* TODO: at least shortdesc *)
+    pr "\n    pub fn %s" name;
+    print_rust_arg_list ~handle:true 2 args;
+    pr " -> %s {\n" ret_rs_type;
+
+    let num = ref 0 in
+    let arg_transform arg num +      (* TODO: transform Rust types to C types
*)
+      match arg with
+      | ArrayAndLen (UInt32 n, len) -> [""], 2
+      | ArrayAndLen _ -> assert false
+      | Bool n -> [], 0
+      | BytesIn (n, _) | BytesPersistIn (n, _) -> [""], 2
+      | BytesPersistOut (n, _) -> [""], 2
+      | BytesOut (_, count) -> [""], 2
+      | Callback (n, _) | CallbackPersist (n, _) -> [""], 2
+      | Flags n -> [""], 1
+      | Int n -> [""], 1
+      | Int64 n -> [""], 1
+      | Mutable arg -> [""], 1
+      | Opaque n -> [""], 1
+      | Path n -> [""], 1
+      | SockAddrAndLen (n, _) -> [""], 2
+      | String n -> [""], 1
+      | StringList n -> [""], 1
+      | UInt n -> [""], 1
+      | UInt32 n -> [""], 1
+      | UInt64 n -> [""], 1
+    in
+
+    let before = List.flatten (List.map (fun arg ->
+        let code, n = arg_transform arg num in
+        num := !num + n;
+        code
+      ) args)
+    in
+
+    let ret_transform ret +      let err_chk +        [(match ret with
+            | RBool
+            | RErr
+            | RFd
+            | RInt
+            | RInt64 -> "if ret == -1 {"
+            | RConstString
+            | RString -> "if ret.is_null() {");
+          "    return Err(NbdError::from_libnbd());";
+          "}";
+        ]
+      in
+      let trans +        match ret with
+        | RBool
+        | RErr
+        | RFd
+        | RInt
+        | RInt64 -> []
+        | RConstString -> [
+            "let ret = unsafe { CStr::from_ptr(ret) };";
+            "let ret = ret.to_str().unwrap();";
+          ]
+        | RString -> [
+            "let c_str = unsafe { CStr::from_ptr(ret as *const c_char)
};";
+            "let ret = c_str.to_string_lossy().into_owned();";
+            "unsafe { libc::free(c_str.as_ptr() as *mut c_void) }; ";
+          ]
+      in
+      let real_ret +        match ret with
+        | RBool -> "ret != 0";
+        | RErr -> "()"
+        | RFd
+        | RInt
+        | RInt64
+        | RConstString
+        | RString -> "ret"
+      in
+      (if may_set_error then err_chk else []) @
+      trans @
+      [(if may_set_error then "Ok(" ^ real_ret ^ ")" else
real_ret)]
+    in
+
+    List.iter (fun str -> pr "        %s\n" str) before;
+    pr "        let ret = unsafe { nbd_%s (self.handle" name;
+    let argnames = List.flatten (List.map name_of_arg args) in
+    List.iter (pr ", %s") argnames;
+    pr ") };\n";
+    List.iter (fun str -> pr "        %s\n" str) (ret_transform
ret);
+    pr "    }\n";
+  in
+
+  generate_header CStyle;
+
+  pr "#[allow(unused_imports)]\n";
+  pr "use std::os::raw::{c_char, c_int, c_uint, c_void};\n";
+  pr "use std::os::unix::io::RawFd;\n";
+  pr "use std::ffi::CStr;\n";
+  pr "use libnbd_sys::*;\n";
+  pr "use libc;\n";
+  pr "\n";
+  pr "pub use super::nbd_error::*;\n";
+  pr "\n";
+
+  pr "pub struct Nbd {\n";
+  pr "    handle: *mut nbd_handle,\n";
+  pr "}\n";
+  pr "\n";
+  pr "impl Nbd {\n";
+  pr "    pub fn create() -> Result<Self, NbdError> {\n";
+  pr "        let handle = unsafe { nbd_create() };\n";
+  pr "        if handle.is_null() {\n";
+  pr "            return Err(NbdError::from_libnbd());\n";
+  pr "        }\n";
+  pr "        Ok(Self { handle: handle })\n";
+  pr "    }\n";
+
+  List.iter print_wrapper handle_calls;
+  pr "}\n";
+
+  pr "impl Drop for Nbd {\n";
+  pr "    fn drop(&mut self) {\n";
+  pr "        unsafe { nbd_close(self.handle);\n";
+  pr "    }\n";
+  pr "}\n"
+
 (*----------------------------------------------------------------------*)
 
 (* Write the output files. *)
@@ -4670,3 +4969,5 @@ let ()    output_to "ocaml/NBD.mli"
generate_ocaml_nbd_mli;
   output_to "ocaml/NBD.ml" generate_ocaml_nbd_ml;
   output_to "ocaml/nbd-c.c" generate_ocaml_nbd_c;
+  output_to "rust/libnbd-sys/src/lib.rs" generate_rust_sys_lib_rs;
+  output_to "rust/libnbd/src/glue.rs" generate_rust_glue_rs;
diff --git a/run.in b/run.in
index 56ed70346922..117ab59d46b8 100755
--- a/run.in
+++ b/run.in
@@ -106,5 +106,14 @@ fi
 export GNOME_KEYRING_CONTROL export GNOME_KEYRING_PID 
+# For rust
+export RUST="@RUST@"
+if [ -z "$RUSTFLAGS" ]; then
+    RUSTFLAGS="-C link-args=-L$b/lib/.libs"
+else
+    RUSTFLAGS="$RUSTFLAGS -C link-args=-L$b/lib/.libs"
+fi
+export RUSTFLAGS
+
 # Run the program.
 exec $libtool $valgrind "$@"
diff --git a/rust/Cargo.toml b/rust/Cargo.toml
new file mode 100644
index 000000000000..877446074a6b
--- /dev/null
+++ b/rust/Cargo.toml
@@ -0,0 +1,6 @@
+[workspace]
+
+members = [
+    "libnbd-sys",
+    "libnbd",
+]
diff --git a/rust/Makefile.am b/rust/Makefile.am
new file mode 100644
index 000000000000..ca2974e09aa2
--- /dev/null
+++ b/rust/Makefile.am
@@ -0,0 +1,42 @@
+# nbd client library in userspace
+# Copyright (C) 2013-2019 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
+
+include $(top_srcdir)/subdir-rules.mk
+
+clean-local:
+	-cargo clean
+
+generator_built = \
+	src/ffi.rs
+
+EXTRA_DIST = \
+	$(generator_built) \
+	Cargo.toml.in
+
+if HAVE_RUST
+
+all: target/release/libnbd-sys.rlib target/release/examples/hello-nbd
+
+target/release/libnbd-sys.rlib:
+	$(top_builddir)/run cargo build --release
+
+target/release/examples/hello-nbd: target/release/libnbd-sys.rlib
+	$(top_builddir)/run cargo build --release --example hello-nbd
+
+TESTS = run-tests
+
+endif HAVE_RUST
diff --git a/rust/libnbd-sys/Cargo.toml.in b/rust/libnbd-sys/Cargo.toml.in
new file mode 100644
index 000000000000..684510fca14b
--- /dev/null
+++ b/rust/libnbd-sys/Cargo.toml.in
@@ -0,0 +1,10 @@
+[package]
+name = "libnbd-sys"
+version = "@VERSION@"
+authors = ["Martin Kletzander <mkletzan@redhat.com>"]
+edition = "2018"
+links   = "nbd"
+build   = "build.rs"
+
+[dependencies]
+libc = "^0.2.58"
diff --git a/rust/libnbd-sys/build.rs b/rust/libnbd-sys/build.rs
new file mode 100644
index 000000000000..acfdf8e57245
--- /dev/null
+++ b/rust/libnbd-sys/build.rs
@@ -0,0 +1,9 @@
+/*
+ * We have to have a build.rs script in order to use `links` in Cargo.toml.
+ * Ideally this should figure out whether we can build with a library installed
+ * in the system and if not, it should add C files to the build and build it
+ * locally or basically build it without the library being installed in the
+ * system.
+ */
+
+fn main() {}
diff --git a/rust/libnbd-sys/src/.gitkeep b/rust/libnbd-sys/src/.gitkeep
new file mode 100644
index 000000000000..e69de29bb2d1
diff --git a/rust/libnbd/Cargo.toml.in b/rust/libnbd/Cargo.toml.in
new file mode 100644
index 000000000000..6ba12963fc08
--- /dev/null
+++ b/rust/libnbd/Cargo.toml.in
@@ -0,0 +1,9 @@
+[package]
+name = "libnbd"
+version = "@VERSION@"
+authors = ["Martin Kletzander <mkletzan@redhat.com>"]
+edition = "2018"
+
+[dependencies]
+libc = "^0.2.58"
+libnbd-sys = { version = "^@VERSION@", path =
"../libnbd-sys" }
diff --git a/rust/libnbd/examples/hello-nbd.rs
b/rust/libnbd/examples/hello-nbd.rs
new file mode 100644
index 000000000000..5d6279088bc7
--- /dev/null
+++ b/rust/libnbd/examples/hello-nbd.rs
@@ -0,0 +1,7 @@
+use libnbd::*;
+
+fn main() -> Result<(), Box<dyn std::error::Error>> {
+    let nbd = Nbd::create()?;
+    println!("libnbd version is {}", nbd.get_version());
+    Ok(())
+}
diff --git a/rust/libnbd/src/lib.rs b/rust/libnbd/src/lib.rs
new file mode 100644
index 000000000000..b69b7c1a1863
--- /dev/null
+++ b/rust/libnbd/src/lib.rs
@@ -0,0 +1,4 @@
+mod nbd_error;
+
+mod glue;
+pub use glue::*;
diff --git a/rust/libnbd/src/nbd_error.rs b/rust/libnbd/src/nbd_error.rs
new file mode 100644
index 000000000000..0a7f667d5ab7
--- /dev/null
+++ b/rust/libnbd/src/nbd_error.rs
@@ -0,0 +1,31 @@
+use libnbd_sys::{nbd_get_errno, nbd_get_error};
+use std::ffi::CStr;
+use std::fmt;
+
+#[derive(Debug, Copy, Clone)]
+pub struct NbdError {
+    errno: i32,
+    strerr: &'static CStr,
+}
+
+impl NbdError {
+    pub fn from_libnbd() -> Self {
+        Self {
+            errno: unsafe { nbd_get_errno() },
+            strerr: unsafe { CStr::from_ptr(nbd_get_error()) },
+        }
+    }
+}
+
+impl std::error::Error for NbdError {}
+
+impl fmt::Display for NbdError {
+    fn fmt(&self, f: &mut fmt::Formatter<'_>) ->
fmt::Result {
+        write!(
+            f,
+            "NBD Error (errno = {}): {}",
+            self.errno,
+            self.strerr.to_string_lossy()
+        )
+    }
+}
diff --git a/rust/run-tests b/rust/run-tests
new file mode 100755
index 000000000000..d1e7b58113e6
--- /dev/null
+++ b/rust/run-tests
@@ -0,0 +1,4 @@
+#!/bin/sh
+set -e
+
+$CARGO test
-- 
2.22.0

The patch seems OK in general.

On Sun, Jul 07, 2019 at 11:39:29PM +0200, Martin Kletzander
wrote:
> The way the code is generated is also not nice, I wish there was
> more code actually written in some files and not generated by the
> generator (as much hard-coded static strings as possible), maybe
> similarly to the states.c, I don't know.
Can you expand on what you mean by this?
> Also up for discussion is whether the libnbd crate should be
> separate since the higher-level functionality it should provide will
> not be tightly coupled with libnbd itself and the releases
> (especially the numbers) do not need to happen in sync.
I also didn't understand what this means.

It's a matter of personal preference but you can use multi-line string
constants in OCaml which can be unlimited in length, so this:
> +  pr "#[allow(unused_imports)]\n";
> +  pr "use std::os::raw::{c_char, c_int, c_uint, c_void};\n";
> +  pr "use std::os::unix::io::RawFd;\n";
> +  pr "use std::ffi::CStr;\n";
> +  pr "use libnbd_sys::*;\n";
> +  pr "use libc;\n";
> +  pr "\n";
(etc)

can be written as:

  pr "\
#[allow(unused_imports)]
use std::os::raw::{c_char, c_int, c_uint, c_void};
use std::os::unix::io::RawFd;
[...]
";

These are still C-like printf-like strings so you still need to escape
%, " and \.

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

On Mon, Jul 08, 2019 at 09:58:20AM +0100, Richard W.M. Jones
wrote:
>The patch seems OK in general.
>
The libnbd-sys part is actually complete.  Well, except the build script, but
that one is not completely necessary.  And the documentation.

The wrappers are missing quite a few things to be usable, but it should not be
*that* difficult once someone wants to play with it.

What bothers me a lot is the way I am composing some of the strings.  The only
way why I shamelessly sent this was that the code runs once to generate the file
and it is not part of the resulting product =)
>On Sun, Jul 07, 2019 at 11:39:29PM +0200, Martin Kletzander wrote:
>> The way the code is generated is also not nice, I wish there was
>> more code actually written in some files and not generated by the
>> generator (as much hard-coded static strings as possible), maybe
>> similarly to the states.c, I don't know.
>
>Can you expand on what you mean by this?
>
For example the `impl` blocks can be combined from different modules/files, so
the implementations that are just constant (not generated dynamically, just
static strings printed out) can be in a separate file instead of being written
every time by the generator.

Maybe some substrings could be identified to be common and then copied from a
file instead of duplicating the data.  But this is not needed.
>> Also up for discussion is whether the libnbd crate should be
>> separate since the higher-level functionality it should provide will
>> not be tightly coupled with libnbd itself and the releases
>> (especially the numbers) do not need to happen in sync.
>
>I also didn't understand what this means.
>
>It's a matter of personal preference but you can use multi-line string
>constants in OCaml which can be unlimited in length, so this:
>
>> +  pr "#[allow(unused_imports)]\n";
>> +  pr "use std::os::raw::{c_char, c_int, c_uint, c_void};\n";
>> +  pr "use std::os::unix::io::RawFd;\n";
>> +  pr "use std::ffi::CStr;\n";
>> +  pr "use libnbd_sys::*;\n";
>> +  pr "use libc;\n";
>> +  pr "\n";
>(etc)
>
>can be written as:
>
>  pr "\
>#[allow(unused_imports)]
>use std::os::raw::{c_char, c_int, c_uint, c_void};
>use std::os::unix::io::RawFd;
>[...]
>";
This makes it much nicer, I noticed it in the generator, but I have not spent
any time figuring out why my editor complained about it.  But I wasted a lot of
time on missing/extra semicolons, so it could've been something silly like
that.

I'll spend some time one it (that is if I find some spare free time in near
future).
>
>These are still C-like printf-like strings so you still need to escape
>%, " and \.
>
>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