Libguestfs - Oct 2017 - [PATCH v2 1/2] v2v: Fix RPM file owned test (RHBZ#1503958).

v1 was here:
https://www.redhat.com/archives/libguestfs/2017-October/msg00183.html

v2:
 - Adds back the Debian test, but simplified.
 - Adds tests on Fedora & Debian.

Rich.

Linux.file_owner is not used by any other function, so remove it.

Linux.is_file_owned is only used when removing kmod-xenpv on old RHEL
releases, and so is only required to work for RPM.

The old file_owner/is_file_owned functions were completely broken for
the RPM case.  This replaces them with a simpler, working
implementation of is_file_owned only.

Thanks: Ming Xie for finding and reporting the original bug.
---
 v2v/linux.ml  | 61 ++++++++++++++++++++++++++---------------------------------
 v2v/linux.mli |  3 ---
 2 files changed, 27 insertions(+), 37 deletions(-)

diff --git a/v2v/linux.ml b/v2v/linux.ml
index bc4af1ad2..c97f03f05 100644
--- a/v2v/linux.ml
+++ b/v2v/linux.ml
@@ -99,7 +99,7 @@ let file_list_of_package (g : Guestfs.guestfs) inspect app    
error (f_"don’t know how to get list of files from package using %s")
       format
 
-let rec file_owner (g : G.guestfs) { i_package_format = package_format } path
+let is_file_owned (g : G.guestfs) { i_package_format = package_format } path   
match package_format with
   | "deb" ->
       (* With dpkg usually the directories are owned by all the packages
@@ -109,48 +109,41 @@ let rec file_owner (g : G.guestfs) { i_package_format =
package_format } path         *)
       let cmd = [| "dpkg"; "-S"; path |] in
       debug "%s" (String.concat " " (Array.to_list cmd));
-      let lines -        try g#command_lines cmd
-        with Guestfs.Error msg as exn ->
-          if String.find msg "no path found matching pattern" >= 0
then
-            raise Not_found
-          else
-            raise exn in
-      if Array.length lines = 0 then
-        error (f_"internal error: file_owner: dpkg command returned no
output");
-      let line = lines.(0) in
-      let line -        try String.sub line 0 (String.rindex line ':')
-        with Invalid_argument _ ->
-          error (f_"internal error: file_owner: invalid dpkg output:
‘%s’")
-                line in
-      fst (String.split "," line)
-
-  | "rpm" ->
-      (* Although it is possible in RPM for multiple packages to own
-       * a file, this deliberately only returns one package.
-       *)
-      let cmd = [| "rpm"; "-qf"; "--qf";
"%{NAME}\\n"; path |] in
-      debug "%s" (String.concat " " (Array.to_list cmd));
       (try
-         let pkgs = g#command_lines cmd in
-         pkgs.(0)
+         let lines = g#command_lines cmd in
+         if Array.length lines = 0 then
+           error (f_"internal error: is_file_owned: dpkg command returned
no output");
+         (* Just check the output looks something like "pkg:
filename". *)
+         if String.find lines.(0) ": " >= 0 then
+           true
+         else
+           error (f_"internal error: is_file_owned: unexpected output from
dpkg command: %s")
+                 lines.(0)
        with Guestfs.Error msg as exn ->
-         if String.find msg "is not owned" >= 0 then
-           raise Not_found
+         if String.find msg "no path found matching pattern" >= 0
then
+           false
          else
            raise exn
-       | Invalid_argument _ (* pkgs.(0) raises index out of bounds *) ->
-         error (f_"internal error: file_owner: rpm command returned no
output")
       )
 
+  | "rpm" ->
+     (* Run rpm -qf and print a magic string if the file is owned.
+      * If not owned, rpm will print "... is not owned by any
package"
+      * and exit with an error.  Unfortunately the string is sent to
+      * stdout, so here we ignore the exit status of rpm and just
+      * look for one of the two strings.
+      *)
+     let magic = "FILE_OWNED_TEST" in
+     let cmd = sprintf "rpm -qf --qf %s %s 2>&1 ||:"
+                       (quote (magic ^ "\n")) (quote path) in
+     let r = g#sh cmd in
+     if String.find r magic >= 0 then true
+     else if String.find r "is not owned" >= 0 then false
+     else failwithf "RPM file owned test failed: %s" r
+
   | format ->
     error (f_"don’t know how to find file owner using %s") format
 
-and is_file_owned g inspect path -  try ignore (file_owner g inspect path);
true
-  with Not_found -> false
-
 let is_package_manager_save_file filename    (* Recognized suffixes of package
managers. *)
   let suffixes = [ ".dpkg-old"; ".dpkg-new";
".rpmsave"; ".rpmnew"; ] in
diff --git a/v2v/linux.mli b/v2v/linux.mli
index 705073644..08146a460 100644
--- a/v2v/linux.mli
+++ b/v2v/linux.mli
@@ -29,9 +29,6 @@ val remove : Guestfs.guestfs -> Types.inspect -> string
list -> unit
 val file_list_of_package : Guestfs.guestfs -> Types.inspect ->
Guestfs.application2 -> string list
 (** Return list of files owned by package. *)
 
-val file_owner : Guestfs.guestfs -> Types.inspect -> string -> string
-(** Return the name of the package that owns a file. *)
-
 val is_file_owned : Guestfs.guestfs -> Types.inspect -> string -> bool
 (** Returns true if the file is owned by an installed package. *)
 
-- 
2.13.2

The previous commit revealed this function was very broken.  This adds
a slow test so that we can detect if it is broken or gets broken in
future.  It tests Fedora and Debian guests.
---
 .gitignore                 |  2 ++
 po/POTFILES-ml             |  1 +
 v2v/Makefile.am            | 57 ++++++++++++++++++++++++++++++++++--
 v2v/v2v-slow-unit-tests.sh | 42 +++++++++++++++++++++++++++
 v2v/v2v_slow_unit_tests.ml | 72 ++++++++++++++++++++++++++++++++++++++++++++++
 5 files changed, 172 insertions(+), 2 deletions(-)

diff --git a/.gitignore b/.gitignore
index 36a193054..823b9e050 100644
--- a/.gitignore
+++ b/.gitignore
@@ -655,6 +655,8 @@ Makefile.in
 /v2v/test-v2v-conversion-of-*.sh
 /v2v/uefi.ml
 /v2v/uefi.mli
+/v2v/v2v-slow-unit-tests-*.img
+/v2v/v2v_slow_unit_tests
 /v2v/v2v_unit_tests
 /v2v/virt-v2v
 /v2v/virt-v2v.1
diff --git a/po/POTFILES-ml b/po/POTFILES-ml
index dbf151ea1..478f71b9b 100644
--- a/po/POTFILES-ml
+++ b/po/POTFILES-ml
@@ -163,6 +163,7 @@ v2v/types.ml
 v2v/uefi.ml
 v2v/utils.ml
 v2v/v2v.ml
+v2v/v2v_slow_unit_tests.ml
 v2v/v2v_unit_tests.ml
 v2v/vCenter.ml
 v2v/windows.ml
diff --git a/v2v/Makefile.am b/v2v/Makefile.am
index 6d90faf00..51f345250 100644
--- a/v2v/Makefile.am
+++ b/v2v/Makefile.am
@@ -21,6 +21,8 @@ EXTRA_DIST = \
 	$(SOURCES_MLI) $(SOURCES_ML) $(SOURCES_C) \
 	copy_to_local.ml \
 	copy_to_local.mli \
+	v2v_slow_unit_tests.ml \
+	v2v-slow-unit-tests.sh \
 	v2v_unit_tests.ml \
 	virt-v2v.pod \
 	virt-v2v-copy-to-local.pod
@@ -305,7 +307,9 @@ TESTS += \
 endif
 
 if HAVE_OCAML_PKG_OUNIT
-TESTS += v2v_unit_tests
+TESTS += \
+	v2v_unit_tests \
+	v2v-slow-unit-tests.sh
 endif
 
 if ENABLE_APPLIANCE
@@ -360,6 +364,10 @@ SLOW_TESTS = \
 	$(real_guests_scripts) \
 	test-v2v-trim.sh
 
+if HAVE_OCAML_PKG_OUNIT
+SLOW_TESTS += v2v-slow-unit-tests.sh
+endif
+
 check-slow:
 	$(MAKE) check TESTS="$(SLOW_TESTS)" SLOW=1
 
@@ -483,7 +491,9 @@ EXTRA_DIST += \
 # Unit tests.
 check_PROGRAMS  if HAVE_OCAML_PKG_OUNIT
-check_PROGRAMS += v2v_unit_tests
+check_PROGRAMS += \
+	v2v_unit_tests \
+	v2v_slow_unit_tests
 endif
 
 v2v_unit_tests_BOBJECTS = \
@@ -528,6 +538,49 @@ v2v_unit_tests_LINK = \
 	  $(OCAMLLINKFLAGS) \
 	  $(v2v_unit_tests_THEOBJECTS) -o $@
 
+v2v_slow_unit_tests_BOBJECTS = \
+	types.cmo \
+	uefi.cmo \
+	utils.cmo \
+	DOM.cmo \
+	create_ovf.cmo \
+	windows.cmo \
+	windows_virtio.cmo \
+	linux.cmo \
+	parse_vmx.cmo \
+	inspect_source.cmo \
+	v2v_slow_unit_tests.cmo
+v2v_slow_unit_tests_XOBJECTS = $(v2v_slow_unit_tests_BOBJECTS:.cmo=.cmx)
+
+v2v_slow_unit_tests_SOURCES = $(virt_v2v_SOURCES)
+v2v_slow_unit_tests_CPPFLAGS = $(virt_v2v_CPPFLAGS)
+v2v_slow_unit_tests_CFLAGS = $(virt_v2v_CFLAGS)
+
+if !HAVE_OCAMLOPT
+# Can't call this v2v_slow_unit_tests_OBJECTS because automake gets
confused.
+v2v_slow_unit_tests_THEOBJECTS = $(v2v_slow_unit_tests_BOBJECTS)
+v2v_slow_unit_tests.cmo: OCAMLPACKAGES += -package oUnit
+else
+v2v_slow_unit_tests_THEOBJECTS = $(v2v_slow_unit_tests_XOBJECTS)
+v2v_slow_unit_tests.cmx: OCAMLPACKAGES += -package oUnit
+endif
+
+v2v_slow_unit_tests_DEPENDENCIES = \
+	$(v2v_slow_unit_tests_THEOBJECTS) \
+	../common/mlstdutils/mlstdutils.$(MLARCHIVE) \
+	../common/mlxml/mlxml.$(MLARCHIVE) \
+	../common/mlgettext/mlgettext.$(MLARCHIVE) \
+	../common/mlpcre/mlpcre.$(MLARCHIVE) \
+	../common/mlutils/mlcutils.$(MLARCHIVE) \
+	../common/mltools/mltools.$(MLARCHIVE) \
+	$(top_srcdir)/ocaml-link.sh
+v2v_slow_unit_tests_LINK = \
+	$(top_srcdir)/ocaml-link.sh -cclib '$(OCAMLCLIBS)' -- \
+	  $(OCAMLFIND) $(BEST) $(OCAMLFLAGS) \
+	  $(OCAMLPACKAGES) -package oUnit \
+	  $(OCAMLLINKFLAGS) \
+	  $(v2v_slow_unit_tests_THEOBJECTS) -o $@
+
 # Dependencies.
 .depend: *.mli *.ml
 	$(top_builddir)/ocaml-dep.sh $^
diff --git a/v2v/v2v-slow-unit-tests.sh b/v2v/v2v-slow-unit-tests.sh
new file mode 100755
index 000000000..1847211db
--- /dev/null
+++ b/v2v/v2v-slow-unit-tests.sh
@@ -0,0 +1,42 @@
+#!/bin/bash -
+# libguestfs virt-v2v test script
+# 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.
+
+# Wrapper around v2v_slow_unit_tests (OCaml test program).
+
+set -e
+
+$TEST_FUNCTIONS
+slow_test
+skip_if_skipped
+
+rm -f v2v-slow-unit-tests-*.img
+
+# These images are used for testing.
+virt-builder fedora-26 \
+             -o v2v-slow-unit-tests-fedora-26.img \
+             --mkdir /tmp/notowned \
+             --touch /tmp/notowned2
+virt-builder debian-9 \
+             -o v2v-slow-unit-tests-debian-9.img \
+             --mkdir /tmp/notowned \
+             --touch /tmp/notowned2
+
+# Run the actual tests.
+./v2v_slow_unit_tests
+
+rm v2v-slow-unit-tests-*.img
diff --git a/v2v/v2v_slow_unit_tests.ml b/v2v/v2v_slow_unit_tests.ml
new file mode 100644
index 000000000..c27cba634
--- /dev/null
+++ b/v2v/v2v_slow_unit_tests.ml
@@ -0,0 +1,72 @@
+(* virt-v2v
+ * 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.
+ *)
+
+(* Test individual virt-v2v utility functions for tests which
+ * are long running.
+ *)
+
+open Printf
+
+open OUnit2
+
+open Std_utils
+open Tools_utils
+
+open Types
+
+let get_g_i filename +  let g = open_guestfs () in
+  g#add_drive filename ~readonly:true;
+  g#launch ();
+  let inspect = Inspect_source.inspect_source SingleRoot g in
+  g, inspect
+
+let fedora_g, fedora_i = get_g_i "v2v-slow-unit-tests-fedora-26.img"
+let debian_g, debian_i = get_g_i "v2v-slow-unit-tests-debian-9.img"
+
+let test_is_file_owned ctx +  let test g i filename expected +    let printer =
string_of_bool in
+    let msg +      sprintf "Linux.is_file_owned %S (expecting %b)"
filename expected in
+    assert_equal ~printer ~msg
+                 expected (Linux.is_file_owned g i filename)
+  in
+
+  test fedora_g fedora_i "/etc/passwd" true;
+  (* /etc/passwd is not in fact owned in Debian. *)
+  test debian_g debian_i "/etc/debian_version" true;
+  test fedora_g fedora_i "/var/log" true;
+  test debian_g debian_i "/var/log" true;
+
+  test fedora_g fedora_i "/tmp/notowned" false;
+  test debian_g debian_i "/tmp/notowned" false;
+  test fedora_g fedora_i "/tmp/notowned2" false;
+  test debian_g debian_i "/tmp/notowned2" false;
+
+  ()
+
+(* Suites declaration. *)
+let suite +  "virt-v2v" >:::
+    [
+      "Linux.is_file_owned" >:: test_is_file_owned;
+    ]
+
+let () +  run_test_tt_main suite
-- 
2.13.2