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

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