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

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

On Fri, Jul 26, 2019 at 06:41:14PM +0200, Pino Toscano wrote:
Hi Hiroyuki,

sorry for the late reply.

Most of the work is definitely nice! There are few notes below,
although they are not big issues.  I will check this patch once more
on monday, especially the rust parts.

Otherwise, I'd say that we are close to merging this :)

On Tuesday, 23 July 2019 10:37:17 CEST 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

IMHO there should be a commit message saying that this is new binding
for Rust and its name, and what is included (actions & tests, not
events, not examples).

Also, as we talked about, make sure to fix the copyright to properly
credit yourself.

 rust/src/.gitkeep                   |   0

This .gitkeep file is not needed, as there are other files in src/.

 rust/tests/.gitkeep                 |   0


+(* Utilities for Rust *)
+(* Are there corresponding functions to them? *)
+(* Should they be placed in utils.ml? *)

Usually we add generic functions to common places only when we know in
advance that there will be multiple users.  Otherwise, like in this
case, having utilities only where used is perfectly fine.
In any case, if any of these utilities will be needed in more places in
the future, it is easy to do a simple no-op commit to just move some

+let rec indent n = match n with
+  | x when x > 0 -> pr "    "; indent (x - 1)
+  | _ -> ()

A small nit here:

let rec indent = function
 | 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))

The generator uses the internal mlstdutils shared code, see
common/mlstdutils.  One of the things provided are extra functions for
the String module, and one in particular can help here: String.nsplit.

+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

... this can be simplified a bit using String.nsplit, and currying:

let snake2caml name =
 let l = String.nsplit "_" name in
 let l = List.map String.capitalize_ascii 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

Hm IMHO the condition in the if sounds like List.mem :) What about:

 if List.mem s black_list then

+      let cname = snake2caml name in
+      let rec contains_ptr args = match args with
+        | [] -> false
+        | OString _ ::_
+        | OStringList _::_ -> true
+        | _::xs -> contains_ptr xs

As above, you can avoid the explicit match on the last parameter using
the function syntax.

OTOH, I think you can use List.exists here:

let contains_ptr =
 List.exists (
   | OString _
   | OStringList _ -> true
   | _ -> false

diff --git a/rust/Cargo.toml.in b/rust/Cargo.toml.in
new file mode 100644
index 000000000..e25dfe768
--- /dev/null
+++ b/rust/Cargo.toml.in
@@ -0,0 +1,6 @@
+name = "guestfs"
+version = "@VERSION@"
+edition = "2018"

From what I remember about the Rust naming conventions, maybe the name
should be guestfs-sys? Martin?

That would be for a crate that does nothing else then just export the C
functions without any code.  This is fine as it is, guestfs is not going to have
two different stages, it probably does not make sense for this library and is
probably not worth the hassle.

+	.gitignore \
+	$(generator_built) \
+	tests/*.rs \
+	Cargo.toml \
+	Cargo.lock \
+	run-bindtests \
+	run-tests

Most probably also src/*.rs, as they are not automake sources (which
are distributed automatically).

The same also for the various .gitkeep files, as they need to be in the
distribution tarball to ensure the directories exist.

Also, you need to use TESTS_ENVIRONMENT like done elsewhere, so the
in-built stuff is used when running the tests.

diff --git a/rust/run-bindtests b/rust/run-bindtests
new file mode 100755
index 000000000..55484a2c7
--- /dev/null
+++ b/rust/run-bindtests
@@ -0,0 +1,23 @@
+#!/bin/sh -
+# libguestfs Rust bindings
+# Copyright (C) 2013 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
+# 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.
+set -e
+$CARGO run --bin bindtests > bindtests.tmp

Hmm I don't think $CARGO is exported.  A simple way is to make it
available is to export it as test environment, via TESTS_ENVIRONMENT.

It is, automatically, thanks to the way it was searched for.  At least it worked
for me.

diff --git a/rust/src/base.rs b/rust/src/base.rs
new file mode 100644
index 000000000..2dfad91a1
--- /dev/null
+++ b/rust/src/base.rs
@@ -0,0 +1,125 @@
+/* libguestfs generated file
+ *          from the code in the generator/ subdirectory.

Definitely not ;-)

Pino Toscano

Attachment: signature.asc
Description: PGP signature

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