Libguestfs - Apr 2018 - [PATCH 0/3] v2v: Miscellaneous refactoring and fixes.

Originally an attempt to fix:
https://bugzilla.redhat.com/show_bug.cgi?id=1570407

However this isn't a complete fix.  The OVA supplied doesn't even
conform to VMware's own "specification" (I use the word loosely). 
The
OVF inside the OVA references the disk.vmdk file, but the OVA doesn't
contain that disk.vmdk file, only a snapshot called
disk.vmdk.000000000.  Therefore to really fix this bug we need some
major work in input_ova.ml.

Nevertheless these changes make sense on their own.

Rich.

Simple code refactoring, making it both more difficult to ignore the
error case and easier to add other error cases in future.
---
 builder/builder.ml           |  9 +++++----
 common/mltools/checksums.ml  | 17 +++++++++++++----
 common/mltools/checksums.mli | 16 +++++++++++-----
 v2v/input_ova.ml             | 10 ++++++----
 4 files changed, 35 insertions(+), 17 deletions(-)

diff --git a/builder/builder.ml b/builder/builder.ml
index 478b41bba..83c7aefed 100644
--- a/builder/builder.ml
+++ b/builder/builder.ml
@@ -313,10 +313,11 @@ let main ()      match entry with
     (* New-style: Using a checksum. *)
     | { Index.checksums = Some csums } ->
-      (try Checksums.verify_checksums csums template
-      with Checksums.Mismatched_checksum (csum, csum_actual) ->
-        error (f_"%s checksum of template did not match the expected
checksum!\n  found checksum: %s\n  expected checksum: %s\nTry:\n - Use the ‘-v’
option and look for earlier error messages.\n - Delete the cache: virt-builder
--delete-cache\n - Check no one has tampered with the website or your
network!")
-          (Checksums.string_of_csum_t csum) csum_actual
(Checksums.string_of_csum csum)
+      (match Checksums.verify_checksums csums template with
+       | Checksums.Good_checksum -> ()
+       | Checksums.Mismatched_checksum (csum, csum_actual) ->
+          error (f_"%s checksum of template did not match the expected
checksum!\n  found checksum: %s\n  expected checksum: %s\nTry:\n - Use the ‘-v’
option and look for earlier error messages.\n - Delete the cache: virt-builder
--delete-cache\n - Check no one has tampered with the website or your
network!")
+                (Checksums.string_of_csum_t csum) csum_actual
(Checksums.string_of_csum csum)
       )
 
     | { Index.checksums = None } ->
diff --git a/common/mltools/checksums.ml b/common/mltools/checksums.ml
index a40edca76..4dd69e734 100644
--- a/common/mltools/checksums.ml
+++ b/common/mltools/checksums.ml
@@ -27,7 +27,9 @@ type csum_t  | SHA256 of string
 | SHA512 of string
 
-exception Mismatched_checksum of (csum_t * string)
+type csum_result +  | Good_checksum
+  | Mismatched_checksum of csum_t * string
 
 let string_of_csum_t = function
   | SHA1 _ -> "sha1"
@@ -73,8 +75,15 @@ let compute_checksum csum_type ?tar filename  let
verify_checksum csum ?tar filename    let csum_type = string_of_csum_t csum in
   let csum_actual = compute_checksum csum_type ?tar filename in
-  if csum <> csum_actual then
-    raise (Mismatched_checksum (csum, (string_of_csum csum_actual)))
+  if csum = csum_actual then
+    Good_checksum
+  else
+    Mismatched_checksum (csum, string_of_csum csum_actual)
 
 let verify_checksums checksums filename -  List.iter (fun c ->
verify_checksum c filename) checksums
+  List.fold_left (
+    fun acc c ->
+      match acc with
+      | Good_checksum -> verify_checksum c filename
+      | Mismatched_checksum _ as acc -> acc
+  ) Good_checksum checksums
diff --git a/common/mltools/checksums.mli b/common/mltools/checksums.mli
index 92336a18b..d45b29dfd 100644
--- a/common/mltools/checksums.mli
+++ b/common/mltools/checksums.mli
@@ -21,7 +21,10 @@ type csum_t  | SHA256 of string
 | SHA512 of string
 
-exception Mismatched_checksum of (csum_t * string) (* expected checksum, got *)
+type csum_result +  | Good_checksum
+  (* expected checksum, actual checksum. *)
+  | Mismatched_checksum of csum_t * string
 
 val of_string : string -> string -> csum_t
 (** [of_string type value] returns the [csum_t] for the specified
@@ -29,14 +32,17 @@ val of_string : string -> string -> csum_t
 
     Raise [Invalid_argument] if the checksum type is not known. *)
 
-val verify_checksum : csum_t -> ?tar:string -> string -> unit
-(** [verify_checksum type filename] Verify the checksum of the file.
+val verify_checksum : csum_t -> ?tar:string -> string -> csum_result
+(** [verify_checksum type filename] verifies the checksum of the file.
 
     When optional [tar] is used it is path to uncompressed tar archive
     and the [filename] is a path in the tar archive. *)
 
-val verify_checksums : csum_t list -> string -> unit
-(** Verify all the checksums of the file. *)
+val verify_checksums : csum_t list -> string -> csum_result
+(** Verify all the checksums of the file.
+
+    If any checksum fails, the first failure (only) is returned in
+    {!csum_result}. *)
 
 val string_of_csum_t : csum_t -> string
 (** Return a string representation of the checksum type. *)
diff --git a/v2v/input_ova.ml b/v2v/input_ova.ml
index a909b92ed..59dbe6f5f 100644
--- a/v2v/input_ova.ml
+++ b/v2v/input_ova.ml
@@ -224,15 +224,17 @@ object
                 and disk = PCRE.sub 2
                 and expected = PCRE.sub 3 in
                 let csum = Checksums.of_string mode expected in
-                try
+                match
                   if partial then
                     Checksums.verify_checksum csum
                                               ~tar:ova (mf_subfolder // disk)
                   else
                     Checksums.verify_checksum csum (mf_folder // disk)
-                with Checksums.Mismatched_checksum (_, actual) ->
-                  error (f_"checksum of disk %s does not match manifest %s
(actual %s(%s) = %s, expected %s(%s) = %s)")
-                        disk mf mode disk actual mode disk expected;
+                with
+                | Checksums.Good_checksum -> ()
+                | Checksums.Mismatched_checksum (_, actual) ->
+                   error (f_"checksum of disk %s does not match manifest
%s (actual %s(%s) = %s, expected %s(%s) = %s)")
+                         disk mf mode disk actual mode disk expected;
               )
               else
                 warning (f_"unable to parse line from manifest file:
%S") line;
-- 
2.16.2

Some OVA files generated by VMware have a *.mf file which contains
checksums for files which don't exist in the OVA.  Ignore these
checksums.

Thanks: Nisim Simsolo.
---
 builder/builder.ml           |  3 +++
 common/mltools/checksums.ml  | 28 +++++++++++++++++++++-------
 common/mltools/checksums.mli |  2 ++
 v2v/input_ova.ml             |  9 ++++++++-
 4 files changed, 34 insertions(+), 8 deletions(-)

diff --git a/builder/builder.ml b/builder/builder.ml
index 83c7aefed..2ed7cb97a 100644
--- a/builder/builder.ml
+++ b/builder/builder.ml
@@ -318,6 +318,9 @@ let main ()         | Checksums.Mismatched_checksum (csum,
csum_actual) ->
           error (f_"%s checksum of template did not match the expected
checksum!\n  found checksum: %s\n  expected checksum: %s\nTry:\n - Use the ‘-v’
option and look for earlier error messages.\n - Delete the cache: virt-builder
--delete-cache\n - Check no one has tampered with the website or your
network!")
                 (Checksums.string_of_csum_t csum) csum_actual
(Checksums.string_of_csum csum)
+       | Checksums.Missing_file ->
+          error (f_"%s: template not downloaded or deleted.  You may have
run ‘virt-builder --delete-cache’ in parallel.")
+                template
       )
 
     | { Index.checksums = None } ->
diff --git a/common/mltools/checksums.ml b/common/mltools/checksums.ml
index 4dd69e734..fdeae1dff 100644
--- a/common/mltools/checksums.ml
+++ b/common/mltools/checksums.ml
@@ -30,6 +30,7 @@ type csum_t  type csum_result    | Good_checksum
   | Mismatched_checksum of csum_t * string
+  | Missing_file
 
 let string_of_csum_t = function
   | SHA1 _ -> "sha1"
@@ -72,18 +73,31 @@ let compute_checksum csum_type ?tar filename      let
csum_str = fst (String.split " " line) in
     of_string csum_type csum_str
 
+(* Check if the direct file exists or if it exists in the tarball. *)
+let file_exists ?tar filename +  match tar with
+  | None -> Sys.file_exists filename
+  | Some tar ->
+     let cmd +       sprintf "tar tf %s %s >/dev/null 2>&1"
(quote tar) (quote filename) in
+     Sys.command cmd = 0
+
 let verify_checksum csum ?tar filename -  let csum_type = string_of_csum_t csum
in
-  let csum_actual = compute_checksum csum_type ?tar filename in
-  if csum = csum_actual then
-    Good_checksum
-  else
-    Mismatched_checksum (csum, string_of_csum csum_actual)
+  if not (file_exists ?tar filename) then
+    Missing_file
+  else (
+    let csum_type = string_of_csum_t csum in
+    let csum_actual = compute_checksum csum_type ?tar filename in
+    if csum = csum_actual then
+      Good_checksum
+    else
+      Mismatched_checksum (csum, string_of_csum csum_actual)
+  )
 
 let verify_checksums checksums filename    List.fold_left (
     fun acc c ->
       match acc with
       | Good_checksum -> verify_checksum c filename
-      | Mismatched_checksum _ as acc -> acc
+      | (Mismatched_checksum _|Missing_file) as acc -> acc
   ) Good_checksum checksums
diff --git a/common/mltools/checksums.mli b/common/mltools/checksums.mli
index d45b29dfd..533e399bf 100644
--- a/common/mltools/checksums.mli
+++ b/common/mltools/checksums.mli
@@ -25,6 +25,8 @@ type csum_result    | Good_checksum
   (* expected checksum, actual checksum. *)
   | Mismatched_checksum of csum_t * string
+  (* referenced file does not exist *)
+  | Missing_file
 
 val of_string : string -> string -> csum_t
 (** [of_string type value] returns the [csum_t] for the specified
diff --git a/v2v/input_ova.ml b/v2v/input_ova.ml
index 59dbe6f5f..f23a1f2a9 100644
--- a/v2v/input_ova.ml
+++ b/v2v/input_ova.ml
@@ -234,7 +234,14 @@ object
                 | Checksums.Good_checksum -> ()
                 | Checksums.Mismatched_checksum (_, actual) ->
                    error (f_"checksum of disk %s does not match manifest
%s (actual %s(%s) = %s, expected %s(%s) = %s)")
-                         disk mf mode disk actual mode disk expected;
+                         disk mf mode disk actual mode disk expected
+                | Checksums.Missing_file ->
+                   (* RHBZ#1570407: Some OVA files generated by VMware
+                    * reference non-existent components in the *.mf file.
+                    * Generate a warning and ignore it.
+                    *)
+                   warning (f_"%s has a checksum for non-existent file %s
(ignored)")
+                           mf disk
               )
               else
                 warning (f_"unable to parse line from manifest file:
%S") line;
-- 
2.16.2

We use ‘tar tRvf’ to parse the locations of files within the tarball.
However examination of tar.git:src/list.c shows various other messages
which can appear in the output:

  block <offset>: ** Block of NULs **
  block <offset>: ** End of File **

Indeed it was easy to produce the first message just by using modern
tar to create a tarball:

  $ tar tRvf '/var/tmp/bz1570407-reproducer.ova'
  block 0: -rw-r--r-- rjones/rjones   100 2018-04-22 17:06
RHEL7_3_042218_extra-disk1.vmdk.000000000
  block 2: -rw-r--r-- rjones/rjones   243 2018-04-22 17:07
RHEL7_3_042218_extra.mf
  block 4: -rw-r--r-- rjones/rjones 13066 2018-04-22 15:08
RHEL7_3_042218_extra.ovf
  block 31: ** Block of NULs **

Ignore these messages.
---
 v2v/utils.ml | 71 ++++++++++++++++++++++++++++++++++--------------------------
 1 file changed, 40 insertions(+), 31 deletions(-)

diff --git a/v2v/utils.ml b/v2v/utils.ml
index 372ad8aaa..d73011f9f 100644
--- a/v2v/utils.ml
+++ b/v2v/utils.ml
@@ -147,6 +147,7 @@ let error_if_no_ssh_agent ()      error (f_"ssh-agent
authentication has not been set up ($SSH_AUTH_SOCK is not set).  This is
required by qemu to do passwordless ssh access.  See the virt-v2v(1) man page
for more information.")
 
 let ws = PCRE.compile "\\s+"
+let re_tar_message = PCRE.compile "\\*\\* [^*]+ \\*\\*$"
 
 let find_file_in_tar tar filename    let lines = external_command (sprintf
"tar tRvf %s" (Filename.quote tar)) in
@@ -156,42 +157,50 @@ let find_file_in_tar tar filename      | line :: lines
-> (
       (* Lines have the form:
        * block <offset>: <perms> <owner>/<group>
<size> <mdate> <mtime> <file>
+       * or:
+       * block <offset>: ** Block of NULs **
+       * block <offset>: ** End of File **
        *)
-      let elems = PCRE.nsplit ~max:8 ws line in
-      if List.length elems = 8 && List.hd elems = "block"
then (
-        let elems = Array.of_list elems in
-        let offset = elems.(1) in
-        let size = elems.(4) in
-        let fname = elems.(7) in
+      if PCRE.matches re_tar_message line then
+        loop lines (* ignore "** Block of NULs **" etc. *)
+      else (
+        let elems = PCRE.nsplit ~max:8 ws line in
+        if List.length elems = 8 && List.hd elems = "block"
then (
+          let elems = Array.of_list elems in
+          let offset = elems.(1) in
+          let size = elems.(4) in
+          let fname = elems.(7) in
 
-        if fname <> filename then
-          loop lines
-        else (
-          let offset -            try
-              (* There should be a colon at the end *)
-              let i = String.rindex offset ':' in
-              if i == (String.length offset)-1 then
-                Int64.of_string (String.sub offset 0 i)
-              else
-                failwith "colon at wrong position"
-            with Failure _ | Not_found ->
-              failwithf (f_"invalid offset returned by tar: %S")
offset in
+          if fname <> filename then
+            loop lines
+          else (
+            let offset +              try
+                (* There should be a colon at the end *)
+                let i = String.rindex offset ':' in
+                if i == (String.length offset)-1 then
+                  Int64.of_string (String.sub offset 0 i)
+                else
+                  failwith "colon at wrong position"
+              with Failure _ | Not_found ->
+                failwithf (f_"invalid offset returned by tar: %S")
offset in
 
-          let size -            try Int64.of_string size
-            with Failure _ ->
-              failwithf (f_"invalid size returned by tar: %S") size
in
+            let size +              try Int64.of_string size
+              with Failure _ ->
+                failwithf (f_"invalid size returned by tar: %S") size
in
 
-          (* Note: Offset is actualy block number and there is a single
-           * block with tar header at the beginning of the file. So skip
-           * the header and convert the block number to bytes before
-           * returning.
-           *)
-          (offset +^ 1L) *^ 512L, size
+            (* Note: Offset is actualy block number and there is a single
+             * block with tar header at the beginning of the file. So skip
+             * the header and convert the block number to bytes before
+             * returning.
+             *)
+            (offset +^ 1L) *^ 512L, size
+          )
         )
-      ) else
-        failwithf (f_"failed to parse line returned by tar: %S") line
+        else
+          failwithf (f_"failed to parse line returned by tar: %S")
line
+      )
     )
   in
   loop lines
-- 
2.16.2