[Libguestfs] [PATCH v4 01/12] v2v: Factor out the nbdkit VDDK code into a new module.

[Martin Kletzander]

On Fri, Sep 20, 2019 at 10:28:12AM +0100, Richard W.M. Jones wrote:
>This refactoring takes the nbdkit-specific code from the ‘-it vddk’
>mode and puts it into a separate module.  The idea is to reuse this
>module (in future commits) to support ssh, curl and rate limiting.
>
>This refactoring is not quite neutral because it moves some of the
>prechecking into the Nbdkit.create function.  This means it will
>happen a little bit later in the cycle (in input#source instead of
>input#precheck - refer to the diagram in v2v/types.mli).  However it's
>still almost in the same place and importantly still happens before we
>start copying.
>---
> v2v/Makefile.am           |   2 +
> v2v/input_libvirt_vddk.ml | 291 ++++++--------------------------------
> v2v/nbdkit.ml             | 280 ++++++++++++++++++++++++++++++++++++
> v2v/nbdkit.mli            |  47 ++++++
> 4 files changed, 374 insertions(+), 246 deletions(-)
>
>diff --git a/v2v/nbdkit.ml b/v2v/nbdkit.ml
>new file mode 100644
>index 000000000..6bf38daa0
>--- /dev/null
>+++ b/v2v/nbdkit.ml
>@@ -0,0 +1,280 @@
>+(* virt-v2v
>+ * 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.
>+ *)
>+
>+open Unix
>+open Printf
>+
>+open Common_gettext.Gettext
>+open Std_utils
>+open Tools_utils
>+open Unix_utils
>+
>+open Utils
>+
>+type t = {
>+  (* The nbdkit plugin name. *)
>+  plugin_name : string;
>+
>+  (* Parameters (includes the plugin name). *)
>+  args : string list;
>+
>+  (* Environment variables that may be needed for nbdkit to work. *)
>+  env : (string * string) list;
>+}
>+
>+(* Check that nbdkit is available and new enough. *)
>+let error_unless_nbdkit_working () =
>+  if 0 <> Sys.command "nbdkit --version >/dev/null" then
>+    error (f_"nbdkit is not installed or not working");
>+
>+  (* Check it's a new enough version.  The latest features we
>+   * require are ‘--exit-with-parent’ and ‘--selinux-label’, both
>+   * added in 1.1.14.  (We use 1.1.16 as the minimum here because
>+   * it also adds the selinux=yes|no flag in --dump-config).
>+   *)
>+  let lines = external_command "nbdkit --help" in
>+  let lines = String.concat " " lines in
>+  if String.find lines "exit-with-parent" == -1 ||
>+     String.find lines "selinux-label" == -1 then
>+    error (f_"nbdkit is not new enough, you need to upgrade to nbdkit ≥ 1.1.16")
>+
>+(* Check that nbdkit was compiled with SELinux support (for the
>+ * --selinux-label option).
>+ *)
>+let error_unless_nbdkit_compiled_with_selinux () =
>+  if have_selinux then (
>+    let lines = external_command "nbdkit --dump-config" in
>+    (* In nbdkit <= 1.1.15 the selinux attribute was not present
>+     * at all in --dump-config output so there was no way to tell.
>+     * Ignore this case because there will be an error later when
>+     * we try to use the --selinux-label parameter.
>+     *)
>+    if List.mem "selinux=no" (List.map String.trim lines) then
>+      error (f_"nbdkit was compiled without SELinux support.  You will have to recompile nbdkit with libselinux-devel installed, or else set SELinux to Permissive mode while doing the conversion.")
>+  )
>+
>+let common_create plugin_name plugin_args plugin_env =
>+  error_unless_nbdkit_working ();
>+  error_unless_nbdkit_compiled_with_selinux ();

Couldn't these (or at least those that do not take any parameters) be checked in
a separate ("precheck" again?) function so that the same behaviour is kept (or
at least partially)?  Even later in the series where you dump the config and use
that as an input for some of the checks, it could be done earlier, right?  Or is
it not the case because it will change based on the environment and that is not
ready earlier?  It has to be if it was part of precheck before, right?

I'm fine with it the way it is, it would just look nicer I think.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/libguestfs/attachments/20190920/baf813d1/attachment.sig>