Libguestfs - Nov 2016 - [PATCH 3/5] v2v: ova: move the untar function

This is still a draft, not ready for commit yet. But feedback is
welcomed. 

This series is related to the problem of inefficient import of OVA
files. The needed enhancements of QEMU was merged into the codebase and
should be available in QEMU 2.8. From there we can use 'size' and
'offset' options in raw driver to tell QEMU to use only subset of a file
as an image.

The first three patches are just preparation. The main work is in patch
four.

There are still some issues that need to be addressed:

1)  The obviously missing check whether QEMU installed in the system
    supports the feature we need. Do we already have such (or similar)
    checks in libguestfs? Should it be compile-time check or runtime
    check? 

2)  Lots of 'tar' invocations that may not be easily understandable.

3)  We need a check for sparse files in tar archive. There is a GNU
    extension to tar format that allows space-efficient archiving of
    sparse files. While the OVF standard does not allow the use of GNU
    extensions we probably should not rely on that.

To address points 2) and 3) I see two possible options. We can either
use some OCaml module that understands tar. There is however a risk that
such module won't provide the necessary info to detect 3). The second
option is to use e.g. libarchive and write small set of C functions that
would implement the necessary magic and use those in OCaml code.
Richard, do you have any preferences or suggestions in this matter?


Tomáš Golembiovský (5):
  mllib: compute checksum of file inside tar
  v2v: ova: don't detect compressed disks, read the OVF instead
  v2v: ova: move the untar function
  v2v: ova: don't extract files from OVA if it's not needed
  v2v: update tests to match changes in OVA import

 mllib/checksums.ml                     |  10 ++-
 mllib/checksums.mli                    |   2 +-
 v2v/Makefile.am                        |   1 +
 v2v/input_ova.ml                       | 144 ++++++++++++++++++++++++++-------
 v2v/test-v2v-i-ova-formats.sh          |   5 +-
 v2v/test-v2v-i-ova-gz.ovf              |   2 +-
 v2v/test-v2v-i-ova-subfolders.expected |   2 +-
 v2v/test-v2v-i-ova-tar.expected        |  18 +++++
 v2v/test-v2v-i-ova-tar.ovf             | 138 +++++++++++++++++++++++++++++++
 v2v/test-v2v-i-ova-tar.sh              |  64 +++++++++++++++
 v2v/test-v2v-i-ova-two-disks.expected  |   4 +-
 v2v/test-v2v-i-ova-two-disks.sh        |   5 +-
 12 files changed, 354 insertions(+), 41 deletions(-)
 create mode 100644 v2v/test-v2v-i-ova-tar.expected
 create mode 100644 v2v/test-v2v-i-ova-tar.ovf
 create mode 100755 v2v/test-v2v-i-ova-tar.sh

-- 
2.10.1

Signed-off-by: Tomáš Golembiovský <tgolembi@redhat.com>
---
 mllib/checksums.ml  | 10 ++++++++--
 mllib/checksums.mli |  2 +-
 2 files changed, 9 insertions(+), 3 deletions(-)

diff --git a/mllib/checksums.ml b/mllib/checksums.ml
index dfa8c3a..9fcff2f 100644
--- a/mllib/checksums.ml
+++ b/mllib/checksums.ml
@@ -45,7 +45,7 @@ let of_string csum_type csum_value    | "sha512"
-> SHA512 csum_value
   | _ -> invalid_arg csum_type
 
-let verify_checksum csum filename +let verify_checksum csum ?(tar =
"") filename    let prog, csum_ref      match csum with
     | SHA1 c -> "sha1sum", c
@@ -53,7 +53,13 @@ let verify_checksum csum filename      | SHA512 c ->
"sha512sum", c
   in
 
-  let cmd = sprintf "%s %s" prog (Filename.quote filename) in
+  let cmd +    if tar = "" then
+      sprintf "%s %s" prog (Filename.quote filename)
+    else
+      sprintf "tar xOf %s %s | %s"
+        (Filename.quote tar) (Filename.quote filename) prog
+  in
   let lines = external_command cmd in
   match lines with
   | [] ->
diff --git a/mllib/checksums.mli b/mllib/checksums.mli
index 0074837..2440324 100644
--- a/mllib/checksums.mli
+++ b/mllib/checksums.mli
@@ -29,7 +29,7 @@ val of_string : string -> string -> csum_t
 
     Raise [Invalid_argument] if the checksum type is not known. *)
 
-val verify_checksum : csum_t -> string -> unit
+val verify_checksum : csum_t -> ?tar:string -> string -> unit
 (** Verify the checksum of the file. *)
 
 val verify_checksums : csum_t list -> string -> unit
-- 
2.10.1

The information whether the disk is gzip compressed or not is stored
in the OVF. There is no reason to do the detection.

Signed-off-by: Tomáš Golembiovský <tgolembi@redhat.com>
---
 v2v/input_ova.ml          | 36 ++++++++++++++++++++----------------
 v2v/test-v2v-i-ova-gz.ovf |  2 +-
 2 files changed, 21 insertions(+), 17 deletions(-)

diff --git a/v2v/input_ova.ml b/v2v/input_ova.ml
index b283629..db884d9 100644
--- a/v2v/input_ova.ml
+++ b/v2v/input_ova.ml
@@ -275,25 +275,29 @@ object
             | None -> error (f_"no href in ovf:File (id=%s)")
file_ref
             | Some s -> s in
 
+          let expr = sprintf
"/ovf:Envelope/ovf:References/ovf:File[@ovf:id='%s']/@ovf:compression"
file_ref in
+          let compressed +            match xpath_string expr with
+            | None | Some "identity" -> false
+            | Some "gzip" -> true
+            | Some s -> error (f_"unsupported comprression in OVF:
%s") s in
+
+          let filename = if compressed then (
+            let new_filename = tmpdir // String.random8 () ^ ".vmdk"
in
+            let cmd +              sprintf "zcat %s > %s" (quote
ovf_folder // filename) (quote new_filename) in
+            if shell_command cmd <> 0 then
+              error (f_"error uncompressing %s, see earlier error
messages")
+                filename;
+            new_filename
+          )
+          else
+            ovf_folder // filename
+          in
+
           (* Does the file exist and is it readable? *)
-          let filename = ovf_folder // filename in
           Unix.access filename [Unix.R_OK];
 
-          (* The spec allows the file to be gzip-compressed, in which case
-           * we must uncompress it into the tmpdir.
-           *)
-          let filename -            if detect_file_type filename = `GZip then (
-              let new_filename = tmpdir // String.random8 () ^
".vmdk" in
-              let cmd -                sprintf "zcat %s > %s"
(quote filename) (quote new_filename) in
-              if shell_command cmd <> 0 then
-                error (f_"error uncompressing %s, see earlier error
messages")
-                  filename;
-              new_filename
-            )
-            else filename in
-
           let disk = {
             s_disk_id = i;
             s_qemu_uri = filename;
diff --git a/v2v/test-v2v-i-ova-gz.ovf b/v2v/test-v2v-i-ova-gz.ovf
index e10ad2b..4a03e85 100644
--- a/v2v/test-v2v-i-ova-gz.ovf
+++ b/v2v/test-v2v-i-ova-gz.ovf
@@ -1,7 +1,7 @@
 <?xml version="1.0" encoding="UTF-8"?>
 <Envelope vmw:buildId="build-1750787"
xmlns="http://schemas.dmtf.org/ovf/envelope/1"
xmlns:cim="http://schemas.dmtf.org/wbem/wscim/1/common"
xmlns:ovf="http://schemas.dmtf.org/ovf/envelope/1"
xmlns:rasd="http://schemas.dmtf.org/wbem/wscim/1/cim-schema/2/CIM_ResourceAllocationSettingData"
xmlns:vmw="http://www.vmware.com/schema/ovf"
xmlns:vssd="http://schemas.dmtf.org/wbem/wscim/1/cim-schema/2/CIM_VirtualSystemSettingData"
xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance">
   <References>
-    <File ovf:href="disk1.vmdk.gz" ovf:id="file1"
ovf:size="7804077568"/>
+    <File ovf:href="disk1.vmdk.gz" ovf:id="file1"
ovf:size="7804077568" ovf:compression="gzip"/>
   </References>
   <DiskSection>
     <Info>Virtual disk information</Info>
-- 
2.10.1

Move the untar function so it can be used later in the code.

Signed-off-by: Tomáš Golembiovský <tgolembi@redhat.com>
---
 v2v/input_ova.ml | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/v2v/input_ova.ml b/v2v/input_ova.ml
index db884d9..f76fe82 100644
--- a/v2v/input_ova.ml
+++ b/v2v/input_ova.ml
@@ -38,6 +38,12 @@ object
   method as_options = "-i ova " ^ ova
 
   method source () +
+    let untar ?(format = "") file outdir +      let cmd = [
"tar"; sprintf "-x%sf" format; file; "-C"; outdir
] in
+      if run_command cmd <> 0 then
+        error (f_"error unpacking %s, see earlier error messages")
ova in
+
     (* Extract ova file. *)
     let exploded        (* The spec allows a directory to be specified as an
ova.  This
@@ -61,11 +67,6 @@ object
 
           tmpfile in
 
-        let untar ?(format = "") file outdir -          let cmd = [
"tar"; sprintf "-x%sf" format; file; "-C"; outdir
] in
-          if run_command cmd <> 0 then
-            error (f_"error unpacking %s, see earlier error
messages") ova in
-
         match detect_file_type ova with
         | `Tar ->
           (* Normal ovas are tar file (not compressed). *)
-- 
2.10.1

We don't have to always extract all files from the OVA archive. The OVA,
as defined in the standard, is plain tar. We can work directly over the
tar archive if we use correct 'offset' and 'size' options when
defining
the backing file for QEMU.

This leads to improvements in speed and puts much lower requirement on
available disk space.

Signed-off-by: Tomáš Golembiovský <tgolembi@redhat.com>
---
 v2v/input_ova.ml | 113 +++++++++++++++++++++++++++++++++++++++++++++++--------
 1 file changed, 98 insertions(+), 15 deletions(-)

diff --git a/v2v/input_ova.ml b/v2v/input_ova.ml
index f76fe82..60dbaf1 100644
--- a/v2v/input_ova.ml
+++ b/v2v/input_ova.ml
@@ -39,17 +39,20 @@ object
 
   method source ()  
-    let untar ?(format = "") file outdir -      let cmd = [
"tar"; sprintf "-x%sf" format; file; "-C"; outdir
] in
+    (* Untar part or all files from tar archive. If [path] is specified it is
+     * a path in the tar archive.
+     *)
+    let untar ?(format = "") ?(path="") file outdir +     
let cmd = [ "tar"; sprintf "-x%sf" format; file;
"-C"; outdir; path ] in
       if run_command cmd <> 0 then
         error (f_"error unpacking %s, see earlier error messages")
ova in
 
     (* Extract ova file. *)
-    let exploded +    let exploded, partial        (* The spec allows a
directory to be specified as an ova.  This
        * is also pretty convenient.
        *)
-      if is_directory ova then ova
+      if is_directory ova then ova, false
       else (
         let uncompress_head zcat file            let cmd = sprintf "%s
%s" zcat (quote file) in
@@ -67,11 +70,19 @@ object
 
           tmpfile in
 
+        (* Untar only ovf and manifest from the archive *)
+        let untar_partial file outdir +          let cmd1 = [ "tar";
"-tf" ; file ] in
+          let cmd2 = [ "grep"; "\\.\\(ovf\\|mf\\)$" ] in
+          let cmd3 = [ "xargs"; "tar"; "-xf" ;
file; "-C"; outdir ] in
+          if shell_command ((stringify_args cmd1) ^ " | " ^
(stringify_args cmd2) ^ " | " ^ (stringify_args cmd3)) <> 0 then
+            error (f_"error unpacking %s, see earlier error
messages") ova in
+
         match detect_file_type ova with
         | `Tar ->
           (* Normal ovas are tar file (not compressed). *)
-          untar ova tmpdir;
-          tmpdir
+          untar_partial ova tmpdir;
+          tmpdir, true
         | `Zip ->
           (* However, although not permitted by the spec, people ship
            * zip files as ova too.
@@ -81,7 +92,7 @@ object
             [ "-j"; "-d"; tmpdir; ova ] in
           if run_command cmd <> 0 then
             error (f_"error unpacking %s, see earlier error
messages") ova;
-          tmpdir
+          tmpdir, false
         | (`GZip|`XZ) as format ->
           let zcat, tar_fmt              match format with
@@ -94,7 +105,7 @@ object
           (match tmpfiletype with
           | `Tar ->
             untar ~format:tar_fmt ova tmpdir;
-            tmpdir
+            tmpdir, false
           | `Zip | `GZip | `XZ | `Unknown ->
             error (f_"%s: unsupported file format\n\nFormats which we
currently understand for '-i ova' are: tar (uncompressed, compress with
gzip or xz), zip") ova
           )
@@ -135,6 +146,49 @@ object
       loop [dir]
     in
 
+    (* Find file in [tar] archive and return at which byte it starts and how
+     * long it is.
+     *)
+    let find_file_in_tar tar filename +      let cmd1 = [ "tar";
"tRvf"; tar ] in
+      let cmd2 = [ "awk"; sprintf
+        "$8 == \"%s\" {print substr($2, 1, index($2,
\":\")-1), $5}"
+        filename ]
+      in
+      let lines +        external_command ((stringify_args cmd1) ^ " |
" ^ (stringify_args cmd2))
+      in
+      if (List.length lines < 1) then
+        raise Not_found
+      else
+        let soffset, ssize = String.split " " (List.hd lines) in
+        let offset +          try Int64.of_string soffset
+          with Failure _ ->
+            error (f_"Invalid offset returned by `tar`: %s") soffset
+        in
+        let size +          try Int64.of_string ssize
+          with Failure _ ->
+            error (f_"Invalid size returend by `tar': %s") ssize
+        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
+    in
+
+    let subfolder folder parent +      if String.is_prefix folder (parent //
"") then
+        let len = String.length parent in
+        String.sub folder (len+1) (String.length folder-len-1)
+      else if folder = parent then
+        ""
+      else
+        assert(false)
+    in
+
     (* Search for the ovf file. *)
     let ovf = find_files exploded ".ovf" in
     let ovf @@ -152,6 +206,7 @@ object
       fun mf ->
         debug "processing manifest %s" mf;
         let mf_folder = Filename.dirname mf in
+        let mf_subfolder = subfolder mf_folder exploded in
         let chan = open_in mf in
         let rec loop ()            let line = input_line chan in
@@ -160,7 +215,11 @@ object
             let disk = Str.matched_group 2 line in
             let expected = Str.matched_group 3 line in
             let csum = Checksums.of_string mode expected in
-            try Checksums.verify_checksum csum (mf_folder // disk)
+            try
+              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;
@@ -283,25 +342,49 @@ object
             | Some "gzip" -> true
             | Some s -> error (f_"unsupported comprression in OVF:
%s") s in
 
-          let filename = if compressed then (
+          let filename, partial = if compressed then (
+            if partial then
+              untar ~path:((subfolder ovf_folder exploded) // filename)
+                ova tmpdir;
             let new_filename = tmpdir // String.random8 () ^ ".vmdk"
in
             let cmd                sprintf "zcat %s > %s" (quote
ovf_folder // filename) (quote new_filename) in
             if shell_command cmd <> 0 then
               error (f_"error uncompressing %s, see earlier error
messages")
                 filename;
-            new_filename
+            new_filename, false
           )
+          else if partial then
+            (subfolder ovf_folder exploded) // filename, partial
           else
-            ovf_folder // filename
+            ovf_folder // filename, partial
           in
 
-          (* Does the file exist and is it readable? *)
-          Unix.access filename [Unix.R_OK];
+          let qemu_uri +            if not partial then (
+              (* Does the file exist and is it readable? *)
+              Unix.access filename [Unix.R_OK];
+              filename
+            )
+            else (
+              let offset, size +                try find_file_in_tar ova
filename
+                with Not_found ->
+                  error (f_"file '%s' not found in the ova")
filename
+              in
+              (* QEMU requires size aligned to 512 bytes. This is safe because
+               * tar also works with 512 byte blocks.
+               *)
+              let size = roundup64 size 512L in
+              sprintf
+                "json:{ \"file\": {
\"driver\":\"raw\", \"offset\":\"%Ld\",
\"size\":\"%Ld\", \"file\": {
\"filename\":\"%s\" } } }"
+                offset size ova
+            )
+          in
 
           let disk = {
             s_disk_id = i;
-            s_qemu_uri = filename;
+            s_qemu_uri = qemu_uri;
             s_format = Some "vmdk";
             s_controller = controller;
           } in
-- 
2.10.1

Signed-off-by: Tomáš Golembiovský <tgolembi@redhat.com>
---
 v2v/Makefile.am                        |   1 +
 v2v/test-v2v-i-ova-formats.sh          |   5 +-
 v2v/test-v2v-i-ova-subfolders.expected |   2 +-
 v2v/test-v2v-i-ova-tar.expected        |  18 +++++
 v2v/test-v2v-i-ova-tar.ovf             | 138 +++++++++++++++++++++++++++++++++
 v2v/test-v2v-i-ova-tar.sh              |  64 +++++++++++++++
 v2v/test-v2v-i-ova-two-disks.expected  |   4 +-
 v2v/test-v2v-i-ova-two-disks.sh        |   5 +-
 8 files changed, 228 insertions(+), 9 deletions(-)
 create mode 100644 v2v/test-v2v-i-ova-tar.expected
 create mode 100644 v2v/test-v2v-i-ova-tar.ovf
 create mode 100755 v2v/test-v2v-i-ova-tar.sh

diff --git a/v2v/Makefile.am b/v2v/Makefile.am
index 5e3c3eb..621ba10 100644
--- a/v2v/Makefile.am
+++ b/v2v/Makefile.am
@@ -258,6 +258,7 @@ TESTS_ENVIRONMENT = $(top_builddir)/run --test
 
 TESTS = \
 	test-v2v-docs.sh \
+	test-v2v-i-ova-tar.sh \
 	test-v2v-i-ova-formats.sh \
 	test-v2v-i-ova-gz.sh \
 	test-v2v-i-ova-subfolders.sh \
diff --git a/v2v/test-v2v-i-ova-formats.sh b/v2v/test-v2v-i-ova-formats.sh
index d113994..ba51440 100755
--- a/v2v/test-v2v-i-ova-formats.sh
+++ b/v2v/test-v2v-i-ova-formats.sh
@@ -22,7 +22,7 @@ unset CDPATH
 export LANG=C
 set -e
 
-formats="tar zip tar-gz tar-xz"
+formats="zip tar-gz tar-xz"
 
 if [ -n "$SKIP_TEST_V2V_I_OVA_FORMATS_SH" ]; then
     echo "$0: test skipped because environment variable is set"
@@ -62,9 +62,6 @@ echo -e "SHA1(disk1.vmdk)= $sha\r" > disk1.mf
 
 for format in $formats; do
     case "$format" in
-        tar)
-            tar -cf test-$format.ova ../test-v2v-i-ova-formats.ovf disk1.vmdk
disk1.mf
-            ;;
         zip)
             zip -r test ../test-v2v-i-ova-formats.ovf disk1.vmdk disk1.mf
             mv test.zip test-$format.ova
diff --git a/v2v/test-v2v-i-ova-subfolders.expected
b/v2v/test-v2v-i-ova-subfolders.expected
index b6fdb07..38aa3b1 100644
--- a/v2v/test-v2v-i-ova-subfolders.expected
+++ b/v2v/test-v2v-i-ova-subfolders.expected
@@ -10,7 +10,7 @@ hypervisor type: vmware
           video: 
           sound: 
 disks:
-	subfolder/disk1.vmdk (vmdk) [scsi]
+	json:{ "file": { "driver":"raw",
"offset":"2048", "size":"10240",
"file": {
"filename":"test-v2v-i-ova-subfolders.d/test.ova" } } }
(vmdk) [scsi]
 removable media:
 	CD-ROM [ide] in slot 0
 NICs:
diff --git a/v2v/test-v2v-i-ova-tar.expected b/v2v/test-v2v-i-ova-tar.expected
new file mode 100644
index 0000000..f63a399
--- /dev/null
+++ b/v2v/test-v2v-i-ova-tar.expected
@@ -0,0 +1,18 @@
+Source guest information (--print-source option):
+
+    source name: 2K8R2EESP1_2_Medium
+hypervisor type: vmware
+         memory: 1073741824 (bytes)
+       nr vCPUs: 1
+   CPU features: 
+       firmware: uefi
+        display: 
+          video: 
+          sound: 
+disks:
+	json:{ "file": { "driver":"raw",
"offset":"9216", "size":"10240",
"file": {
"filename":"test-v2v-i-ova-tar.d/test-tar.ova" } } } (vmdk)
[scsi]
+removable media:
+	CD-ROM [ide] in slot 0
+NICs:
+	Network "Network adapter 1"
+
diff --git a/v2v/test-v2v-i-ova-tar.ovf b/v2v/test-v2v-i-ova-tar.ovf
new file mode 100644
index 0000000..4827c7e
--- /dev/null
+++ b/v2v/test-v2v-i-ova-tar.ovf
@@ -0,0 +1,138 @@
+<?xml version="1.0" encoding="UTF-8"?>
+<Envelope vmw:buildId="build-1750787"
xmlns="http://schemas.dmtf.org/ovf/envelope/1"
xmlns:cim="http://schemas.dmtf.org/wbem/wscim/1/common"
xmlns:ovf="http://schemas.dmtf.org/ovf/envelope/1"
xmlns:rasd="http://schemas.dmtf.org/wbem/wscim/1/cim-schema/2/CIM_ResourceAllocationSettingData"
xmlns:vmw="http://www.vmware.com/schema/ovf"
xmlns:vssd="http://schemas.dmtf.org/wbem/wscim/1/cim-schema/2/CIM_VirtualSystemSettingData"
xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance">
+  <References>
+    <File ovf:href="disk1.vmdk" ovf:id="file1"
ovf:size="7804077568"/>
+  </References>
+  <DiskSection>
+    <Info>Virtual disk information</Info>
+    <Disk ovf:capacity="50" ovf:capacityAllocationUnits="byte
* 2^30" ovf:diskId="vmdisk1" ovf:fileRef="file1"
ovf:format="http://www.vmware.com/interfaces/specifications/vmdk.html#streamOptimized"
ovf:populatedSize="18975752192"/>
+  </DiskSection>
+  <NetworkSection>
+    <Info>The list of logical networks</Info>
+    <Network ovf:name="PG-VLAN60">
+      <Description>The PG-VLAN60 network</Description>
+    </Network>
+  </NetworkSection>
+  <VirtualSystem ovf:id="2K8R2EESP1_2_Medium">
+    <Info>A virtual machine</Info>
+    <Name>2K8R2EESP1_2_Medium</Name>
+    <OperatingSystemSection ovf:id="103"
vmw:osType="windows7Server64Guest">
+      <Info>The kind of installed guest operating system</Info>
+      <Description>Microsoft Windows Server 2008 R2
(64-bit)</Description>
+    </OperatingSystemSection>
+    <VirtualHardwareSection>
+      <Info>Virtual hardware requirements</Info>
+      <System>
+        <vssd:ElementName>Virtual Hardware
Family</vssd:ElementName>
+        <vssd:InstanceID>0</vssd:InstanceID>
+       
<vssd:VirtualSystemIdentifier>2K8R2EESP1_2_Medium</vssd:VirtualSystemIdentifier>
+        <vssd:VirtualSystemType>vmx-10</vssd:VirtualSystemType>
+      </System>
+      <Item>
+        <rasd:AllocationUnits>hertz * 10^6</rasd:AllocationUnits>
+        <rasd:Description>Number of Virtual CPUs</rasd:Description>
+        <rasd:ElementName>1 virtual CPU(s)</rasd:ElementName>
+        <rasd:InstanceID>1</rasd:InstanceID>
+        <rasd:ResourceType>3</rasd:ResourceType>
+        <rasd:VirtualQuantity>1</rasd:VirtualQuantity>
+      </Item>
+      <Item>
+        <rasd:AllocationUnits>byte * 2^20</rasd:AllocationUnits>
+        <rasd:Description>Memory Size</rasd:Description>
+        <rasd:ElementName>1024MB of memory</rasd:ElementName>
+        <rasd:InstanceID>2</rasd:InstanceID>
+        <rasd:ResourceType>4</rasd:ResourceType>
+        <rasd:VirtualQuantity>1024</rasd:VirtualQuantity>
+      </Item>
+      <Item>
+        <rasd:Address>0</rasd:Address>
+        <rasd:Description>SCSI Controller</rasd:Description>
+        <rasd:ElementName>SCSI controller 0</rasd:ElementName>
+        <rasd:InstanceID>3</rasd:InstanceID>
+        <rasd:ResourceSubType>lsilogicsas</rasd:ResourceSubType>
+        <rasd:ResourceType>6</rasd:ResourceType>
+        <vmw:Config ovf:required="false"
vmw:key="slotInfo.pciSlotNumber" vmw:value="160"/>
+      </Item>
+      <Item>
+        <rasd:Address>1</rasd:Address>
+        <rasd:Description>IDE Controller</rasd:Description>
+        <rasd:ElementName>IDE 1</rasd:ElementName>
+        <rasd:InstanceID>4</rasd:InstanceID>
+        <rasd:ResourceType>5</rasd:ResourceType>
+      </Item>
+      <Item>
+        <rasd:Address>0</rasd:Address>
+        <rasd:Description>IDE Controller</rasd:Description>
+        <rasd:ElementName>IDE 0</rasd:ElementName>
+        <rasd:InstanceID>5</rasd:InstanceID>
+        <rasd:ResourceType>5</rasd:ResourceType>
+      </Item>
+      <Item ovf:required="false">
+        <rasd:AutomaticAllocation>false</rasd:AutomaticAllocation>
+        <rasd:ElementName>Video card</rasd:ElementName>
+        <rasd:InstanceID>6</rasd:InstanceID>
+        <rasd:ResourceType>24</rasd:ResourceType>
+        <vmw:Config ovf:required="false"
vmw:key="enable3DSupport" vmw:value="false"/>
+        <vmw:Config ovf:required="false"
vmw:key="use3dRenderer" vmw:value="automatic"/>
+        <vmw:Config ovf:required="false"
vmw:key="useAutoDetect" vmw:value="true"/>
+        <vmw:Config ovf:required="false"
vmw:key="videoRamSizeInKB" vmw:value="4096"/>
+      </Item>
+      <Item ovf:required="false">
+        <rasd:AutomaticAllocation>false</rasd:AutomaticAllocation>
+        <rasd:ElementName>VMCI device</rasd:ElementName>
+        <rasd:InstanceID>7</rasd:InstanceID>
+        <rasd:ResourceSubType>vmware.vmci</rasd:ResourceSubType>
+        <rasd:ResourceType>1</rasd:ResourceType>
+        <vmw:Config ovf:required="false"
vmw:key="allowUnrestrictedCommunication"
vmw:value="false"/>
+        <vmw:Config ovf:required="false"
vmw:key="slotInfo.pciSlotNumber" vmw:value="32"/>
+      </Item>
+      <Item ovf:required="false">
+        <rasd:AddressOnParent>0</rasd:AddressOnParent>
+        <rasd:AutomaticAllocation>false</rasd:AutomaticAllocation>
+        <rasd:ElementName>CD/DVD drive 1</rasd:ElementName>
+        <rasd:InstanceID>8</rasd:InstanceID>
+        <rasd:Parent>4</rasd:Parent>
+       
<rasd:ResourceSubType>vmware.cdrom.atapi</rasd:ResourceSubType>
+        <rasd:ResourceType>15</rasd:ResourceType>
+      </Item>
+      <Item>
+        <rasd:AddressOnParent>0</rasd:AddressOnParent>
+        <rasd:ElementName>Hard disk 1</rasd:ElementName>
+        <rasd:HostResource>ovf:/disk/vmdisk1</rasd:HostResource>
+        <rasd:InstanceID>9</rasd:InstanceID>
+        <rasd:Parent>3</rasd:Parent>
+        <rasd:ResourceType>17</rasd:ResourceType>
+        <vmw:Config ovf:required="false"
vmw:key="backing.writeThrough" vmw:value="false"/>
+      </Item>
+      <Item>
+        <rasd:AddressOnParent>7</rasd:AddressOnParent>
+        <rasd:AutomaticAllocation>true</rasd:AutomaticAllocation>
+        <rasd:Connection>PG-VLAN60</rasd:Connection>
+        <rasd:Description>E1000 ethernet adapter on
&quot;PG-VLAN60&quot;</rasd:Description>
+        <rasd:ElementName>Network adapter 1</rasd:ElementName>
+        <rasd:InstanceID>11</rasd:InstanceID>
+        <rasd:ResourceSubType>E1000</rasd:ResourceSubType>
+        <rasd:ResourceType>10</rasd:ResourceType>
+        <vmw:Config ovf:required="false"
vmw:key="slotInfo.pciSlotNumber" vmw:value="33"/>
+        <vmw:Config ovf:required="false"
vmw:key="wakeOnLanEnabled" vmw:value="true"/>
+      </Item>
+      <vmw:Config ovf:required="false"
vmw:key="cpuHotAddEnabled" vmw:value="false"/>
+      <vmw:Config ovf:required="false"
vmw:key="cpuHotRemoveEnabled" vmw:value="false"/>
+      <vmw:Config ovf:required="false"
vmw:key="firmware" vmw:value="efi"/>
+      <vmw:Config ovf:required="false"
vmw:key="virtualICH7MPresent" vmw:value="false"/>
+      <vmw:Config ovf:required="false"
vmw:key="virtualSMCPresent" vmw:value="false"/>
+      <vmw:Config ovf:required="false"
vmw:key="memoryHotAddEnabled" vmw:value="false"/>
+      <vmw:Config ovf:required="false"
vmw:key="nestedHVEnabled" vmw:value="false"/>
+      <vmw:Config ovf:required="false"
vmw:key="powerOpInfo.powerOffType" vmw:value="soft"/>
+      <vmw:Config ovf:required="false"
vmw:key="powerOpInfo.resetType" vmw:value="soft"/>
+      <vmw:Config ovf:required="false"
vmw:key="powerOpInfo.standbyAction"
vmw:value="checkpoint"/>
+      <vmw:Config ovf:required="false"
vmw:key="powerOpInfo.suspendType" vmw:value="hard"/>
+      <vmw:Config ovf:required="false"
vmw:key="tools.afterPowerOn" vmw:value="true"/>
+      <vmw:Config ovf:required="false"
vmw:key="tools.afterResume" vmw:value="true"/>
+      <vmw:Config ovf:required="false"
vmw:key="tools.beforeGuestShutdown" vmw:value="true"/>
+      <vmw:Config ovf:required="false"
vmw:key="tools.beforeGuestStandby" vmw:value="true"/>
+      <vmw:Config ovf:required="false"
vmw:key="tools.syncTimeWithHost" vmw:value="false"/>
+      <vmw:Config ovf:required="false"
vmw:key="tools.toolsUpgradePolicy"
vmw:value="upgradeAtPowerCycle"/>
+    </VirtualHardwareSection>
+  </VirtualSystem>
+</Envelope>                                 
diff --git a/v2v/test-v2v-i-ova-tar.sh b/v2v/test-v2v-i-ova-tar.sh
new file mode 100755
index 0000000..6a34570
--- /dev/null
+++ b/v2v/test-v2v-i-ova-tar.sh
@@ -0,0 +1,64 @@
+#!/bin/bash -
+# libguestfs virt-v2v test script
+# Copyright (C) 2014-2016 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 -i ova option with ova file compressed in different ways
+
+unset CDPATH
+export LANG=C
+set -e
+
+if [ -n "$SKIP_TEST_V2V_I_OVA_FORMATS_SH" ]; then
+    echo "$0: test skipped because environment variable is set"
+    exit 77
+fi
+
+if [ "$(guestfish get-backend)" = "uml" ]; then
+    echo "$0: test skipped because UML backend does not support
network"
+    exit 77
+fi
+
+export VIRT_TOOLS_DATA_DIR="$srcdir/../test-data/fake-virt-tools"
+
+. $srcdir/../test-data/guestfs-hashsums.sh
+
+d=test-v2v-i-ova-tar.d
+rm -rf $d
+mkdir $d
+
+pushd $d
+
+# Create a phony OVA.  This is only a test of source parsing, not
+# conversion, so the contents of the disks doesn't matter.
+truncate -s 10k disk1.vmdk
+sha=`do_sha1 disk1.vmdk`
+echo -e "SHA1(disk1.vmdk)= $sha\r" > disk1.mf
+tar -cf test-tar.ova ../test-v2v-i-ova-tar.ovf disk1.vmdk disk1.mf
+
+popd
+
+# Run virt-v2v but only as far as the --print-source stage, and
+# normalize the output.
+$VG virt-v2v --debug-gc --quiet \
+    -i ova $d/test-tar.ova \
+    --print-source |
+sed 's,[^ \t]*\(disk.*.vmdk\),\1,' > $d/source
+
+# Check the parsed source is what we expect.
+diff -u test-v2v-i-ova-tar.expected $d/source
+
+rm -rf $d
diff --git a/v2v/test-v2v-i-ova-two-disks.expected
b/v2v/test-v2v-i-ova-two-disks.expected
index cc850a7..57d6240 100644
--- a/v2v/test-v2v-i-ova-two-disks.expected
+++ b/v2v/test-v2v-i-ova-two-disks.expected
@@ -10,8 +10,8 @@ hypervisor type: vmware
           video: 
           sound: 
 disks:
-	disk1.vmdk (vmdk) [scsi]
-	disk2.vmdk (vmdk) [scsi]
+	json:{ "file": { "driver":"raw",
"offset":"9728", "size":"10240",
"file": {
"filename":"test-v2v-i-ova-two-disks.d/test.ova" } } }
(vmdk) [scsi]
+	json:{ "file": { "driver":"raw",
"offset":"21504", "size":"102400",
"file": {
"filename":"test-v2v-i-ova-two-disks.d/test.ova" } } }
(vmdk) [scsi]
 removable media:
 	CD-ROM [ide] in slot 0
 NICs:
diff --git a/v2v/test-v2v-i-ova-two-disks.sh b/v2v/test-v2v-i-ova-two-disks.sh
index aefd90e..168315e 100755
--- a/v2v/test-v2v-i-ova-two-disks.sh
+++ b/v2v/test-v2v-i-ova-two-disks.sh
@@ -59,8 +59,9 @@ popd
 # normalize the output.
 $VG virt-v2v --debug-gc --quiet \
     -i ova $d/test.ova \
-    --print-source |
-sed 's,[^ \t]*\(disk.*.vmdk\),\1,' > $d/source
+    --print-source  > $d/source # |
+# TODO: this looks strange and should be fixed in some other commit
+#sed 's,[^ \t]*\(disk.*.vmdk\),\1,'
 
 # Check the parsed source is what we expect.
 diff -u test-v2v-i-ova-two-disks.expected $d/source
-- 
2.10.1

On Friday, 4 November 2016 14:52:49 CET Tomáš Golembiovský
wrote:
> Signed-off-by: Tomáš Golembiovský <tgolembi@redhat.com>
> ---
>  mllib/checksums.ml  | 10 ++++++++--
>  mllib/checksums.mli |  2 +-
>  2 files changed, 9 insertions(+), 3 deletions(-)
> 
> diff --git a/mllib/checksums.ml b/mllib/checksums.ml
> index dfa8c3a..9fcff2f 100644
> --- a/mllib/checksums.ml
> +++ b/mllib/checksums.ml
> @@ -45,7 +45,7 @@ let of_string csum_type csum_value >    |
"sha512" -> SHA512 csum_value
>    | _ -> invalid_arg csum_type
>  
> -let verify_checksum csum filename > +let verify_checksum csum ?(tar =
"") filename
In this case, do not set a default value to the optional argument, so
you can later check whether it was actually set. Optional arguments
are handled as 'a option. So ...
>    let prog, csum_ref >      match csum with
>      | SHA1 c -> "sha1sum", c
> @@ -53,7 +53,13 @@ let verify_checksum csum filename >      | SHA512 c
-> "sha512sum", c
>    in
>  
> -  let cmd = sprintf "%s %s" prog (Filename.quote filename) in
> +  let cmd > +    if tar = "" then
> +      sprintf "%s %s" prog (Filename.quote filename)
> +    else
> +      sprintf "tar xOf %s %s | %s"
> +        (Filename.quote tar) (Filename.quote filename) prog
> +  in
... this case be:

  let cmd     match tar with
    | None ->
      sprintf "%s %s" prog (Filename.quote filename)
    | Some tar ->
      sprintf "tar xOf %s %s | %s"
        (Filename.quote tar) (Filename.quote filename) prog in

Thanks,
-- 
Pino Toscano

On Friday, 4 November 2016 14:52:52 CET Tomáš Golembiovský
wrote:
> We don't have to always extract all files from the OVA archive. The
OVA,
> as defined in the standard, is plain tar. We can work directly over the
> tar archive if we use correct 'offset' and 'size' options
when defining
> the backing file for QEMU.
> 
> This leads to improvements in speed and puts much lower requirement on
> available disk space.
> 
> Signed-off-by: Tomáš Golembiovský <tgolembi@redhat.com>
> ---
Just few notes, still need to review the actual bulk of the work done
by this patch.
>  v2v/input_ova.ml | 113
+++++++++++++++++++++++++++++++++++++++++++++++--------
>  1 file changed, 98 insertions(+), 15 deletions(-)
> 
> diff --git a/v2v/input_ova.ml b/v2v/input_ova.ml
> index f76fe82..60dbaf1 100644
> --- a/v2v/input_ova.ml
> +++ b/v2v/input_ova.ml
> @@ -39,17 +39,20 @@ object
>  
>    method source () >  
> -    let untar ?(format = "") file outdir > -      let cmd = [
"tar"; sprintf "-x%sf" format; file; "-C"; outdir
] in
> +    (* Untar part or all files from tar archive. If [path] is specified it
is
> +     * a path in the tar archive.
> +     *)
> +    let untar ?(format = "") ?(path="") file outdir 
The default value for 'path' can be omitted here, checking whether
~path was actually specified in the 'untar' call:
> +      let cmd = [ "tar"; sprintf "-x%sf" format; file;
"-C"; outdir; path ] in
  let cmd     [ "tar"; sprintf "-x%sf" format; file;
"-C"; outdir; ]
    @ (match path with None -> [] | Some p -> [p]) in

In this case, having an empty default value for 'path' means an empty
parameter will be passed as argument for 'tar'.
>        if run_command cmd <> 0 then
>          error (f_"error unpacking %s, see earlier error
messages") ova in
>  
>      (* Extract ova file. *)
> -    let exploded > +    let exploded, partial >        (* The spec
allows a directory to be specified as an ova.  This
>         * is also pretty convenient.
>         *)
> -      if is_directory ova then ova
> +      if is_directory ova then ova, false
>        else (
>          let uncompress_head zcat file >            let cmd = sprintf
"%s %s" zcat (quote file) in
> @@ -67,11 +70,19 @@ object
>  
>            tmpfile in
>  
> +        (* Untar only ovf and manifest from the archive *)
> +        let untar_partial file outdir > +          let cmd1 = [
"tar"; "-tf" ; file ] in
> +          let cmd2 = [ "grep"; "\\.\\(ovf\\|mf\\)$" ]
in
> +          let cmd3 = [ "xargs"; "tar"; "-xf"
; file; "-C"; outdir ] in
> +          if shell_command ((stringify_args cmd1) ^ " | " ^
(stringify_args cmd2) ^ " | " ^ (stringify_args cmd3)) <> 0 then
Hmm... maybe it's just taste, but I find this command concatenation
confusing, and prone to break. I'd use:
1) external_command to get the list of files in the archive
2) filter the filenames (see Filename.check_suffix) to pick only the
   needed ones
3) maybe using the helper 'untar' function above to extract the files
> +            error (f_"error unpacking %s, see earlier error
messages") ova in
> +
>          match detect_file_type ova with
>          | `Tar ->
>            (* Normal ovas are tar file (not compressed). *)
> -          untar ova tmpdir;
> -          tmpdir
> +          untar_partial ova tmpdir;
> +          tmpdir, true
>          | `Zip ->
>            (* However, although not permitted by the spec, people ship
>             * zip files as ova too.
> @@ -81,7 +92,7 @@ object
>              [ "-j"; "-d"; tmpdir; ova ] in
>            if run_command cmd <> 0 then
>              error (f_"error unpacking %s, see earlier error
messages") ova;
> -          tmpdir
> +          tmpdir, false
>          | (`GZip|`XZ) as format ->
>            let zcat, tar_fmt >              match format with
> @@ -94,7 +105,7 @@ object
>            (match tmpfiletype with
>            | `Tar ->
>              untar ~format:tar_fmt ova tmpdir;
> -            tmpdir
> +            tmpdir, false
>            | `Zip | `GZip | `XZ | `Unknown ->
>              error (f_"%s: unsupported file format\n\nFormats which we
currently understand for '-i ova' are: tar (uncompressed, compress with
gzip or xz), zip") ova
>            )
> @@ -135,6 +146,49 @@ object
>        loop [dir]
>      in
>  
> +    (* Find file in [tar] archive and return at which byte it starts and
how
> +     * long it is.
> +     *)
> +    let find_file_in_tar tar filename > +      let cmd1 = [
"tar"; "tRvf"; tar ] in
> +      let cmd2 = [ "awk"; sprintf
> +        "$8 == \"%s\" {print substr($2, 1, index($2,
\":\")-1), $5}"
> +        filename ]
> +      in
As above, I'd use external_command + manual filtering code in OCaml.
YMMV though.
> +      let lines > +        external_command ((stringify_args cmd1) ^
" | " ^ (stringify_args cmd2))
> +      in
> +      if (List.length lines < 1) then
No need for parenthesis, and it can be even simplified as:
  if lines = [] then
> +        raise Not_found
> +      else
> +        let soffset, ssize = String.split " " (List.hd lines) in
> +        let offset > +          try Int64.of_string soffset
> +          with Failure _ ->
> +            error (f_"Invalid offset returned by `tar`: %s")
soffset
> +        in
> +        let size > +          try Int64.of_string ssize
> +          with Failure _ ->
> +            error (f_"Invalid size returend by `tar': %s")
ssize
Typo in the error message, and different wrt the above one (`tar` vs
`tar') .
> +        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
> +    in
> +
> +    let subfolder folder parent > +      if String.is_prefix folder
(parent // "") then
> +        let len = String.length parent in
> +        String.sub folder (len+1) (String.length folder-len-1)
> +      else if folder = parent then
> +        ""
> +      else
> +        assert(false)
No need for parenthesis.



-- 
Pino Toscano

On Friday, 4 November 2016 14:52:51 CET Tomáš Golembiovský
wrote:
> Move the untar function so it can be used later in the code.
> 
> Signed-off-by: Tomáš Golembiovský <tgolembi@redhat.com>
> ---
Code motion only indeed, LGTM.

Thanks,
-- 
Pino Toscano

On Friday, 4 November 2016 14:52:48 CET Tomáš Golembiovský
wrote:
> 1)  The obviously missing check whether QEMU installed in the system
>     supports the feature we need. Do we already have such (or similar)
>     checks in libguestfs? Should it be compile-time check or runtime
>     check?
- yes, we have version checks for qemu, but only internally in the
library (and it does not make sense to export)

- runtime check indeed -- for example something like the following
(taken & simplified from virt-sysprep in the rhel-6.x branches):

  let qemu_img_version ()     let cmd = "qemu-img --version" in
    let lines = external_command cmd in
    match lines with
    | [] -> error ("'qemu-img --version' returned no
output")
    | line :: _ ->
      try
        sscanf line "qemu-img version %d.%d" (
          fun major minor ->
            let minor = if minor > 9 then 9 else minor in
            float major +. float minor /. 10.
        )
      with
        Scan_failure msg ->
          warning (f_"warning: failed to read qemu-img version\n  line:
%S\n  message: %s\n%!")
            line msg;
          0.9
> 2)  Lots of 'tar' invocations that may not be easily
understandable.
Left some comments about them.
> 3)  We need a check for sparse files in tar archive. There is a GNU
>     extension to tar format that allows space-efficient archiving of
>     sparse files. While the OVF standard does not allow the use of GNU
>     extensions we probably should not rely on that.
Is there a way to detect such kind of files in archives, falling back
to usual "extract everything" way as done currently.
> To address points 2) and 3) I see two possible options. We can either
> use some OCaml module that understands tar. There is however a risk that
> such module won't provide the necessary info to detect 3). The second
> option is to use e.g. libarchive and write small set of C functions that
> would implement the necessary magic and use those in OCaml code.
> Richard, do you have any preferences or suggestions in this matter?
That's a workable way too, IMHO.

Thanks,
-- 
Pino Toscano

On Mon, 07 Nov 2016 15:03:36 +0100
Pino Toscano <ptoscano@redhat.com> wrote:
> On Friday, 4 November 2016 14:52:48 CET Tomáš Golembiovský wrote:
> > 1)  The obviously missing check whether QEMU installed in the system
> >     supports the feature we need. Do we already have such (or similar)
> >     checks in libguestfs? Should it be compile-time check or runtime
> >     check?  
> 
> - yes, we have version checks for qemu, but only internally in the
> library (and it does not make sense to export)
> 
> - runtime check indeed -- for example something like the following
> (taken & simplified from virt-sysprep in the rhel-6.x branches):
>
Thanks. I'll reuse that.
>   let qemu_img_version () >     let cmd = "qemu-img --version"
in
>     let lines = external_command cmd in
>     match lines with
>     | [] -> error ("'qemu-img --version' returned no
output")
>     | line :: _ ->
>       try
>         sscanf line "qemu-img version %d.%d" (
>           fun major minor ->
>             let minor = if minor > 9 then 9 else minor in
>             float major +. float minor /. 10.
Uhm. I know this is easy to compare with, but why not return a tuple?
QEMU 2.10 will be here sooner than we can imagine.
>         )
>       with
>         Scan_failure msg ->
>           warning (f_"warning: failed to read qemu-img version\n 
line: %S\n  message: %s\n%!")
>             line msg;
>           0.9
> 
> > 2)  Lots of 'tar' invocations that may not be easily
understandable.
> 
> Left some comments about them.
I will try to rewrite that and do the filtering in OCaml.
> 
> > 3)  We need a check for sparse files in tar archive. There is a GNU
> >     extension to tar format that allows space-efficient archiving of
> >     sparse files. While the OVF standard does not allow the use of GNU
> >     extensions we probably should not rely on that.  
> 
> Is there a way to detect such kind of files in archives, falling back
> to usual "extract everything" way as done currently.
Sorry, somehow I left out this important piece of information. I didn't
find a way to get this information neither with GNU tar utility nor with
bsdtar. I didn't look at the OCaml tar module too closely yet, so I
don't know if this is possible. libarchive on the other hand is able to
tell us whether the file is sparse or not.
> > To address points 2) and 3) I see two possible options. We can either
> > use some OCaml module that understands tar. There is however a risk
that
> > such module won't provide the necessary info to detect 3). The
second
> > option is to use e.g. libarchive and write small set of C functions
that
> > would implement the necessary magic and use those in OCaml code.
> > Richard, do you have any preferences or suggestions in this matter?  
> 
> That's a workable way too, IMHO.
> 
> Thanks,
> -- 
> Pino Toscano

-- 
Tomáš Golembiovský <tgolembi@redhat.com>