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

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(a)gmail.com&gt; > > 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 Ditto. > +(* 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 code. > +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. Also... > +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 ( function | 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 @@ > +[package] > +name = "guestfs" > +version = "@VERSION@" > +edition = "2018" From what I remember about the Rust naming conventions, maybe the name should be guestfs-sys? Martin?

> +EXTRA_DIST = \ > + .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 > +# 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. > + > +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.

> 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 > + * WARNING: THIS FILE IS GENERATED > + * from the code in the generator/ subdirectory. > + * ANY CHANGES YOU MAKE TO THIS FILE WILL BE LOST. Definitely not ;-) -- Pino Toscano