[Libguestfs] [PATCH 2/5] Make sure every *.ml file has a corresponding *.mli file.

Pino Toscano ptoscano at redhat.com
Tue Sep 19 14:31:43 UTC 2017


On Monday, 18 September 2017 18:39:40 CEST Richard W.M. Jones wrote:
>  dib/output_format_docker.mli                       |  19 +
>  dib/output_format_qcow2.mli                        |  19 +
>  dib/output_format_raw.mli                          |  19 +
>  dib/output_format_squashfs.mli                     |  19 +
>  dib/output_format_tar.mli                          |  19 +
>  dib/output_format_tgz.mli                          |  19 +
>  dib/output_format_vhd.mli                          |  19 +
> [...]
>  sysprep/sysprep_operation_abrt_data.mli            |  19 +
>  sysprep/sysprep_operation_backup_files.mli         |  19 +
>  sysprep/sysprep_operation_bash_history.mli         |  19 +
>  sysprep/sysprep_operation_blkid_tab.mli            |  19 +
>  sysprep/sysprep_operation_ca_certificates.mli      |  19 +
>  sysprep/sysprep_operation_crash_data.mli           |  19 +
>  sysprep/sysprep_operation_cron_spool.mli           |  19 +
>  sysprep/sysprep_operation_customize.mli            |  19 +
>  sysprep/sysprep_operation_dhcp_client_state.mli    |  19 +
>  sysprep/sysprep_operation_dhcp_server_state.mli    |  19 +
>  sysprep/sysprep_operation_dovecot_data.mli         |  19 +
>  sysprep/sysprep_operation_firewall_rules.mli       |  19 +
>  sysprep/sysprep_operation_flag_reconfiguration.mli |  19 +
>  sysprep/sysprep_operation_fs_uuids.mli             |  19 +
>  sysprep/sysprep_operation_kerberos_data.mli        |  19 +
>  sysprep/sysprep_operation_logfiles.mli             |  19 +
>  sysprep/sysprep_operation_lvm_uuids.mli            |  19 +
>  sysprep/sysprep_operation_machine_id.mli           |  19 +
>  sysprep/sysprep_operation_mail_spool.mli           |  19 +
>  sysprep/sysprep_operation_net_hostname.mli         |  19 +
>  sysprep/sysprep_operation_net_hwaddr.mli           |  19 +
>  sysprep/sysprep_operation_pacct_log.mli            |  19 +
>  .../sysprep_operation_package_manager_cache.mli    |  19 +
>  sysprep/sysprep_operation_pam_data.mli             |  19 +
>  sysprep/sysprep_operation_passwd_backups.mli       |  19 +
>  sysprep/sysprep_operation_puppet_data_log.mli      |  19 +
>  .../sysprep_operation_rh_subscription_manager.mli  |  19 +
>  sysprep/sysprep_operation_rhn_systemid.mli         |  19 +
>  sysprep/sysprep_operation_rpm_db.mli               |  19 +
>  sysprep/sysprep_operation_samba_db_log.mli         |  19 +
>  sysprep/sysprep_operation_script.mli               |  19 +
>  sysprep/sysprep_operation_smolt_uuid.mli           |  19 +
>  sysprep/sysprep_operation_ssh_hostkeys.mli         |  19 +
>  sysprep/sysprep_operation_ssh_userdir.mli          |  19 +
>  sysprep/sysprep_operation_sssd_db_log.mli          |  19 +
>  sysprep/sysprep_operation_tmp_files.mli            |  19 +
>  sysprep/sysprep_operation_udev_persistent_net.mli  |  19 +
>  sysprep/sysprep_operation_user_account.mli         |  19 +
>  sysprep/sysprep_operation_utmp.mli                 |  19 +
>  sysprep/sysprep_operation_yum_uuid.mli             |  19 +

Would it be possible to generate the files above automatically, during
build?  After all, they all are empty interfaces.

> +val xdg_cache_home : string option
> +(** [$XDG_CACHE_HOME/virt-builder] or [$HOME/.cache/virt-builder] or [None]. *)

(Note to self: I guess this should better use "prog" too, so this
module can be moved to a generic place, should other application need
it.)

> diff --git a/common/mlstdutils/libdir.mli b/common/mlstdutils/libdir.mli
> new file mode 100644
> index 000000000..3bdb42c4e
> --- /dev/null
> +++ b/common/mlstdutils/libdir.mli
> @@ -0,0 +1,20 @@
> +(* mlstdutils
> + * Copyright (C) 2017 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.
> + *)
> +
> +val libdir : string
> +(** The configure value [@libdir@] (fully expanded into a path). *)

I had this doubt when talking with Cédric about his
virt-builder-repository patches: is this module actually used anywhere?
It doesn't look so, so I would just drop it instead.

> diff --git a/dib/elements.mli b/dib/elements.mli
> new file mode 100644
> index 000000000..2413ac1fc
> --- /dev/null
> +++ b/dib/elements.mli
> @@ -0,0 +1,34 @@
> +(* virt-dib
> + * Copyright (C) 2015 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.
> + *)
> +
> +(** Parsing and handling of elements. *)
> +
> +type element = {
> +  directory : string;

(** directory of the element *)

> +  hooks : hooks_map;

(** available hooks, and scripts for each hook *)

> +}
> +and hooks_map = (string, string list) Hashtbl.t (** hook name, scripts *)
> +
> +val builtin_elements_blacklist : string list
> +val builtin_scripts_blacklist : string list

The above have documentation comments in the .ml, I guess the comments
can be moved here.

> +val load_elements : debug:int -> string list -> (string, element) Hashtbl.t

(** [load_elements ~debug paths] loads elements from the specified
    [paths]; returns a [Hashtbl.t] of {!element} structs indexed by
    the element name. *)

> +val load_dependencies : StringSet.elt list -> (StringSet.elt, element) Hashtbl.t -> StringSet.t

I think the second parameter ought to be "(string, element) Hashtbl.t",
i.e. the same that load_elements returned (could be made a type, eg
"element_map").

(** [load_dependencies element_set elements] returns the whole set of
    elements needed to use [element_set], including [element_list]
    themselves.  In other words, this recursively resolves the
    dependencies of [element_set]. *)

> +val copy_elements : StringSet.t -> (StringSet.elt, element) Hashtbl.t -> string list -> string -> unit

(** [copy_elements element_set elements blacklisted_scripts destdir]
    copies the elements in [element_set] (with the element definitions
    provided as [elements]) into the [destdir] directory.

    [blacklisted_scripts] contains names of scripts to never copy. *)

> +val load_hooks : debug:int -> string -> (string, string list) Hashtbl.t

The return value is hooks_map, actually.

(** [load_hooks ~debug path] loads the hooks from the specified
    [path] (which usually represents an element). *)

> +val load_scripts : Guestfs.guestfs -> string -> string list

(** [load_scripts g path] loads the scripts from the specified [path]
    (which usually represents a directory of an hook). *)

> diff --git a/dib/utils.mli b/dib/utils.mli
> new file mode 100644
> index 000000000..3a17a4d88
> --- /dev/null
> +++ b/dib/utils.mli
> @@ -0,0 +1,63 @@
> +(* virt-dib
> + * Copyright (C) 2015 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.
> + *)
> +
> +val unit_GB : int -> int64
> +(** [unit_GB n] returns n * 2^30 *)
> +
> +val current_arch : unit -> string
> +(** Turn the host_cpu into the dpkg architecture naming. *)
> +
> +val output_filename : string -> string -> string
> +(** [output_filename image_name format] generates a suitable output
> +    filename based on the image filename and output format. *)
> +
> +val log_filename : unit -> string
> +(** Generate a name for the log file containing the program name and
> +    current date/time. *)
> +
> +val var_from_lines : string -> string list -> string
> +(** Find variable definition in a set of lines of the form [var=value]. *)
> +
> +val string_index_fn : (char -> bool) -> string -> int
> +(** Apply function to each character in the string.  If the function
> +    returns true, return the index of the character.
> +
> +    @raise Not_found if no match *)

Or "Like {String.index}, but using a function instead of a single
character".

> +val do_mkdir : string -> unit
> +(** Wrapper around [mkdir -p]. *)

(** Wrapper around [mkdir -p 0755]. *)

The rest seems fine.

Thanks,
-- 
Pino Toscano
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: This is a digitally signed message part.
URL: <http://listman.redhat.com/archives/libguestfs/attachments/20170919/d1734768/attachment.sig>


More information about the Libguestfs mailing list