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

[Richard W.M. Jones]

On Mon, Jan 30, 2017 at 10:43:16PM +0100, Tomáš Golembiovský wrote:
> +    QV=$(expr match "$(qemu-img --version)" 'qemu-img version \([0-9]\+\.[0-9]\+\)')

Maybe consider using bash's internal regexp functions?  We already
use them in many other places, try doing:

$ git grep '=~' -- \*.sh

> diff --git a/v2v/input_ova.ml b/v2v/input_ova.ml
> index 40f723633..01ba80686 100644
> --- a/v2v/input_ova.ml
> +++ b/v2v/input_ova.ml
> @@ -39,17 +39,23 @@ 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 [paths] is specified it is
> +     * a list of paths in the tar archive.
> +     *)
> +    let untar ?(format = "") ?paths file outdir =
> +      let cmd =
> +        [ "tar"; sprintf "-x%sf" format; file; "-C"; outdir ]
> +        @ (match paths with None -> [] | Some p -> p)

I don't think you need parens around the match statement here(?)

> +        let qemu_img_version () =
> +          let lines = external_command "qemu-img --version" in
> +          match lines with
> +          | [] -> error ("'qemu-img --version' returned no output")
> +          | line :: _ ->
> +              let rex = Str.regexp
> +                "qemu-img version \\([0-9]+\\)\\.\\([0-9]+\\)" in
> +              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;
> +                  0, 9
> +              ) else (
> +                warning (f_"failed to read qemu-img version(%S), assuming 0.9")
> +                  line;
> +                0, 9
> +              )
> +        in

I can't put my finger on exactly why, but I'm unhappy about this
function.  I think one improvement would be to move it to
v2v/utils.ml.  It clutters up the code being buried here, when really
it's a utility function.

> +    (* Find file in [tar] archive and return at which byte it starts and how
> +     * long it is.
> +     *)
> +    let find_file_in_tar tar filename =

This function is saying "utility" to me as well.

An advantage of putting these functions into utils.ml is that they
will have a properly defined and typed interface (in utils.mli) so
that people won't need to understand exactly how they work in order to
work out the inputs and outputs.

> +    let subfolder folder parent =
> +      if folder = parent then
> +        ""
> +      else if String.is_prefix folder (parent // "") then
> +        let len = String.length parent in
> +        String.sub folder (len+1) (String.length folder-len-1)
> +      else
> +        assert false
> +    in

And I think Cedric already mentions this is a utility function.  He
has a use for it elsewhere, so wants it in mllib/common_utils.ml.

>             * we must uncompress it into the tmpdir.
> +          let qemu_uri =
> +            if not partial then (
> +              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
> +              let doc = [
> +                "file", JSON.Dict [
> +                  "driver", JSON.String "raw";
> +                  "offset", JSON.Int64 offset;
> +                  "size", JSON.Int64 size;
> +                  "file", JSON.Dict [
> +                    "filename", JSON.String ova]
> +                  ]
> +                ] in
> +              let x =
> +              sprintf "json:%s" (JSON.string_of_doc ~fmt:Compact doc)
> +              in

A few style issues in this code.

Put "in" on a line by itself if you're defining a function.

Put "in" on the previous line if you're defining a value.  As both
offset, size and x are values, move "in" to the previous line.
Also the sprintf "json:..." should be indented.

> +              (debug "json: %s" x; x)

You don't need the parens here, and probably it's clearer to put the
return value on the next line.

> -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"
> @@ -63,9 +63,6 @@ cp ../test-v2v-i-ova-formats.ovf .
>  
>  for format in $formats; do
>      case "$format" in
> -        tar)
> -            tar -cf test-$format.ova test-v2v-i-ova-formats.ovf disk1.vmdk disk1.mf
> -            ;;

Is dropping this test necessary?


---

Patch looks good overall.

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-top is 'top' for virtual machines.  Tiny program with many
powerful monitoring features, net stats, disk stats, logging, etc.
http://people.redhat.com/~rjones/virt-top