[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
participants (1)
-
Richard W.M. Jones