[Libguestfs] [PATCH v4 6/6] v2v: ova: don't extract files from OVA if it's not needed

[Pino Toscano]

On Sunday, 18 December 2016 23:16:33 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 puts much lower requirement on available
> disk space.
> 
> Since the virt-v2v behaviour for OVA input now depends on QEMU version
> available this affects some of the tests. The affected tests will have
> two *.expect files and the expected result also has to depend on the
> QEMU used.
> 
> Signed-off-by: Tomáš Golembiovský <tgolembi at redhat.com>
> ---

I cannot find anything big wrong at a first glance -- few notes below.

> diff --git a/v2v/Makefile.am b/v2v/Makefile.am
> index cbb974f..433c295 100644
> --- a/v2v/Makefile.am
> +++ b/v2v/Makefile.am
> @@ -257,6 +257,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 \

The new test needs to be added to EXTRA_DIST below (see lines around
340).

> diff --git a/v2v/input_ova.ml b/v2v/input_ova.ml
> index b116be8..c3fc911 100644
> --- a/v2v/input_ova.ml
> +++ b/v2v/input_ova.ml
> +        let qemu_img_version () =
> +          let cmd = "qemu-img --version" in
> +          let lines = external_command cmd in

'cmd' can be avoided (like done right above, in untar_metadata).

> +          match lines with
> +          | [] -> error ("'qemu-img --version' returned no output")
> +          | line :: _ ->
> +              let rex = Str.regexp "qemu-img version \\([0-9]+\\)\\.\\([0-9]+\\)" in

Too long line, can you please wrap it?

> +              if Str.string_match rex line 0 then (
> +                try
> +                  int_of_string (Str.matched_group 1 line),
> +                  int_of_string (Str.matched_group 2 line)
> +                with Failure _ ->
> +                  warning (f_"failed to parse qemu-img version(%S), assuming 0.9")
> +                  line;

'line' needs to be indented by a couple of spaces more (like done
below).

> +          if qmajor > 2 || (qmajor == 2 && qminor >= 8) then (
> +            (* If QEMU is recent enough we don't have to extract everything. We

I'd start the "We" directly in the second line of the comment.

> +    let find_file_in_tar tar filename =
> +      let lines = external_command (sprintf "tar tRvf %s" (Filename.quote tar)) in

Too long line.

> +          if (List.length elems) = 8 && (List.hd elems) = "block" then (

Most probably the round brackets are not needed in these cases.

> +              sprintf
> +                "json:{ \"file\": { \"driver\":\"raw\", \"offset\":\"%Ld\", \"size\":\"%Ld\", \"file\": { \"filename\":\"%s\" } } }"
> +                offset size ova

This could use the JSON module (in mllib) to create this JSON; also
please check whether the filename needs escaping and/or
qemu_input_filename (see its description in Common_utils).

-- 
Pino Toscano
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: This is a digitally signed message part.
URL: <http://listman.redhat.com/archives/libguestfs/attachments/20170104/d6160f76/attachment.sig>