[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [Libguestfs] [PATCH] Rust bindings: Add Rust bindings



> Is this just trying if the guestfs can be linked with?

Yes. In OCaml bindings, there is the corresponding test(https://github.com/libguestfs/libguestfs/blob/master/ocaml/t/guestfs_010_load.ml). I just mimicked it. If it is not required, I will remove it.

divided the generated files and handmade files in rust/src/ directory. I'll send this fixed patch to this mailing list.

I'm not sure about the license problems. Can you teach me that?

Regards,
Hiroyuki

2019年7月17日(水) 22:40 Martin Kletzander <mkletzan redhat com>:
On Wed, Jul 17, 2019 at 06:49:39PM +0900, Hiroyuki Katsura wrote:
>From: Hiroyuki_Katsura <hiroyuki katsura 0513 gmail com>
>
>Rust bindings: Add create / close functions
>
>Rust bindings: Add 4 bindings tests
>
>Rust bindings: Add generator of structs
>
>Rust bindings: Add generator of structs for optional arguments
>
>Rust bindings: Add generator of function signatures
>
>Rust bindings: Complete actions
>
>Rust bindings: Fix memory management
>
>Rust bindings: Add bindtests
>
>Rust bindings: Add additional 4 bindings tests
>
>Rust bindings: Format test files
>
>Rust bindings: Incorporate bindings to build system
>---
> Makefile.am                         |   3 +
> configure.ac                        |   6 +
> generator/Makefile.am               |   3 +
> generator/bindtests.ml              |  66 +++
> generator/bindtests.mli             |   1 +
> generator/main.ml                   |   5 +
> generator/rust.ml                   | 806 ++++++++++++++++++++++++++++
> generator/rust.mli                  |  22 +
> m4/guestfs-rust.m4                  |  33 ++
> run.in                              |   9 +
> rust/.gitignore                     |   3 +
> rust/Cargo.toml.in                  |   6 +
> rust/Makefile.am                    |  42 ++
> rust/run-bindtests                  |  23 +
> rust/run-tests                      |  21 +
> rust/src/.gitkeep                   |   0
> rust/src/bin/.gitkeep               |   0
> rust/tests/.gitkeep                 |   0
> rust/tests/010_load.rs              |  24 +
> rust/tests/020_create.rs            |  24 +
> rust/tests/030_create_flags.rs      |  29 +
> rust/tests/040_create_multiple.rs   |  38 ++
> rust/tests/050_handle_properties.rs |  62 +++
> rust/tests/070_opt_args.rs          |  41 ++
> rust/tests/080_version.rs           |  26 +
> rust/tests/090_ret_values.rs        |  61 +++
> rust/tests/100_launch.rs            |  65 +++
> 27 files changed, 1419 insertions(+)
> create mode 100644 generator/rust.ml
> create mode 100644 generator/rust.mli
> create mode 100644 m4/guestfs-rust.m4
> create mode 100644 rust/.gitignore
> create mode 100644 rust/Cargo.toml.in
> create mode 100644 rust/Makefile.am
> create mode 100755 rust/run-bindtests
> create mode 100755 rust/run-tests
> create mode 100644 rust/src/.gitkeep
> create mode 100644 rust/src/bin/.gitkeep
> create mode 100644 rust/tests/.gitkeep
> create mode 100644 rust/tests/010_load.rs
> create mode 100644 rust/tests/020_create.rs
> create mode 100644 rust/tests/030_create_flags.rs
> create mode 100644 rust/tests/040_create_multiple.rs
> create mode 100644 rust/tests/050_handle_properties.rs
> create mode 100644 rust/tests/070_opt_args.rs
> create mode 100644 rust/tests/080_version.rs
> create mode 100644 rust/tests/090_ret_values.rs
> create mode 100644 rust/tests/100_launch.rs
>
>diff --git a/generator/rust.ml b/generator/rust.ml
>new file mode 100644
>index 000000000..b7bc76da8
>--- /dev/null
>+++ b/generator/rust.ml
>@@ -0,0 +1,806 @@
>+(* libguestfs
>+ * Copyright (C) 2019 Red Hat Inc.
>+ *
>+ * This program is free software; you can redistribute it and/or modify
>+ * it under the terms of the GNU General Public License as published by
>+ * the Free Software Foundation; either version 2 of the License, or
>+ * (at your option) any later version.
>+ *
>+ * This program 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 General Public License for more details.
>+ *
>+ * You should have received a copy of the GNU General Public License
>+ * along with this program; if not, write to the Free Software
>+ * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA
>+*)
>+
>+(* Please read generator/README first. *)
>+
>+open Std_utils
>+open Types
>+open Utils
>+open Pr
>+open Docstrings
>+open Optgroups
>+open Actions
>+open Structs
>+open C
>+open Events
>+
>+(* Utilities for Rust *)
>+(* Are there corresponding functions to them? *)
>+(* Should they be placed in utils.ml? *)
>+let rec indent n = match n with
>+  | x when x > 0 -> pr "    "; indent (x - 1)
>+  | _ -> ()
>+
>+(* split_on_char exists since OCaml 4.04 *)
>+(* but current requirements: >=4.01 *)
>+let split_on_char c = Str.split (Str.regexp (String.make 1 c))
>+
>+let snake2caml name =
>+  let l = split_on_char '_' name in
>+  let l = List.map (fun x -> String.capitalize_ascii x) l in
>+  String.concat "" l
>+
>+(* because there is a function which contains 'unsafe' field *)
>+let black_list = ["unsafe"]
>+
>+let translate_bad_symbols s =
>+  if List.exists (fun x -> s = x) black_list then
>+    s ^ "_"
>+  else
>+    s
>+
>+let generate_rust () =
>+  generate_header CStyle LGPLv2plus;
>+
>+  pr "

I started with this as well (for the libnbd bindings), but it was a PITA for me
to modify parts of the generator.  I then realized that I found out two things,
each one should help you move part of this outside the generator:

 1) you can have impls for the same struct in different files

 2) you can scope the `pub`, for example `pub(crate)` would make the definition
    public but only for this particular crate.  That way you can share things
    between modules without exposing it to consumers of this crate

>+use std::collections;
>+use std::convert;
>+use std::convert::TryFrom;
>+use std::ffi;
>+use std::os::raw::{c_char, c_int, c_void};
>+use std::ptr;
>+use std::slice;
>+use std::str;
>+
>+#[allow(non_camel_case_types)]
>+enum guestfs_h {} // opaque struct
>+

You should not use empty enums for ffi opaque structs as they can be optimized
out.  Also #[repr(C)] it.

You should rather have a struct with an empty member:

  struct guestfs_handle {
    _unused: [u32; 0],
  }

I'll try to find the official info in rust docs, I just do not know on which
docs page it was.

/me goes looking

Oh yeah, it was in the nomicon:

  https://doc.rust-lang.org/nomicon/ffi.html#representing-opaque-structs

It will be even nicer when RFC 1861 gets stabilized, but that's long long in the
future, I guess:

  https://github.com/rust-lang/rust/issues/43467

Otherwise it looks very similar to what I started, so that should be good
(hopefully) =)

I hope I'll get some more time to go over the whole posting here and try it out
as well.

[...]

>diff --git a/rust/Makefile.am b/rust/Makefile.am
>new file mode 100644
>index 000000000..ff284ec00
>--- /dev/null
>+++ b/rust/Makefile.am
>@@ -0,0 +1,42 @@
>+# libguestfs golang bindings

golang? =)

>+# Copyright (C) 2019 Red Hat Inc.

I'll let Rich figure out what is supposed to be here :)

>diff --git a/rust/run-bindtests b/rust/run-bindtests
>new file mode 100755
>index 000000000..2986e898d
>--- /dev/null
>+++ b/rust/run-bindtests
>@@ -0,0 +1,23 @@
>+#!/bin/sh -
>+# libguestfs Golang bindings
>+# Copyright (C) 2013 Red Hat Inc.

dtto

and some other files as well

[...]

>diff --git a/rust/tests/010_load.rs b/rust/tests/010_load.rs
>new file mode 100644
>index 000000000..4cb43f2c1
>--- /dev/null
>+++ b/rust/tests/010_load.rs
>@@ -0,0 +1,24 @@
>+/* libguestfs Rust bindings
>+Copyright (C) 2009-2019 Red Hat Inc.
>+
>+This program is free software; you can redistribute it and/or modify
>+it under the terms of the GNU General Public License as published by
>+the Free Software Foundation; either version 2 of the License, or
>+(at your option) any later version.
>+
>+This program 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 General Public License for more details.
>+
>+You should have received a copy of the GNU General Public License
>+along with this program; if not, write to the Free Software
>+Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA.
>+*/
>+
>+extern crate guestfs;
>+
>+#[test]
>+fn load() {
>+    // nop
>+}

Is this just trying if the guestfs can be linked with?

>diff --git a/rust/tests/020_create.rs b/rust/tests/020_create.rs
>new file mode 100644
>index 000000000..017dbbac0
>--- /dev/null
>+++ b/rust/tests/020_create.rs
>@@ -0,0 +1,24 @@
>+/* libguestfs Rust bindings
>+Copyright (C) 2009-2019 Red Hat Inc.
>+
>+This program is free software; you can redistribute it and/or modify
>+it under the terms of the GNU General Public License as published by
>+the Free Software Foundation; either version 2 of the License, or
>+(at your option) any later version.
>+
>+This program 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 General Public License for more details.
>+
>+You should have received a copy of the GNU General Public License
>+along with this program; if not, write to the Free Software
>+Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA.
>+*/
>+
>+extern crate guestfs;
>+
>+#[test]
>+fn create() {
>+    assert!(!guestfs::Handle::create().is_err(), "create fail");

Isn't Result.is_ok() same as !Result.is_err()?  Maybe even unwrap() would work
here as it would actually show the error that was returned in the backtrace.

[...]

>diff --git a/rust/tests/050_handle_properties.rs b/rust/tests/050_handle_properties.rs
>new file mode 100644
>index 000000000..0b955d5cf
>--- /dev/null
>+++ b/rust/tests/050_handle_properties.rs
>@@ -0,0 +1,62 @@
>+/* libguestfs Rust bindings
>+Copyright (C) 2009-2019 Red Hat Inc.
>+
>+This program is free software; you can redistribute it and/or modify
>+it under the terms of the GNU General Public License as published by
>+the Free Software Foundation; either version 2 of the License, or
>+(at your option) any later version.
>+
>+This program 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 General Public License for more details.
>+
>+You should have received a copy of the GNU General Public License
>+along with this program; if not, write to the Free Software
>+Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA.

Oh, you're also using the older version of the license here (with the old
"logistics" mentioned), but I guess that's fine.  I, for one, would ban these,
but it's a recommendation to put them in each file (although I think it's an
outdated one, but you never know with lawyers), so...
</rant>

Anyway, it looks good, although I just skimmed it and I haven't tested it.

Martin

[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]