[Libguestfs] [PATCH] v2v: adding input -i ova

[Richard W.M. Jones]

On Thu, Aug 21, 2014 at 01:50:18PM +0100, Richard W.M. Jones wrote:
> +    (* extract ova (tar) file *)
> +    let cmd = sprintf ("tar -xf %s -C %s") (ova) (dir) in

Lots of extra parentheses here :-) The same command can be written
more naturally without any of them:

   let cmd = sprintf "tar -xf %s -C %s" ova dir in

However I think what you might have meant is to call the `quote'
function to safely quote arguments, so that you're not adding security
holes through unescaped input, ie:

   let cmd = sprintf "tar -xf %s -C %s" (quote ova) (quote dir) in

Also I don't think uncompressing into the OVA directory is a good
idea.  However it works for now until we work out how and if we are
able to avoid copying those large disk images unnecessarily.

> +    if Sys.command (cmd) <> 0 then

You don't need to put parentheses around command line arguments:

   if Sys.command cmd <> 0 then

In functional languages, function application is written:

  f a b c

meaning call function `f' with 3 parameters `a', `b' and `c'.

Function application binds tightest, so:

  f a b c + 3

means (f a b c) + 3

> +      error (f_"error running command: %s") cmd
> +        exit 1;

The error function already calls exit (see mllib/common_utils.ml).

What you're actually doing here is exploiting a bug in ocaml-gettext
where it prevents the compiler from correctly detecting extra
parameters passed to a printf-like function.  Just delete "exit 1"
(but not the semicolon).  Same thing several times later on too.

> +    let files = Sys.readdir(dir) in

No parens needed around function parameters.

> +    let mf = ref "" in
> +    let ovf = ref "" in
> +    (* search for the ovf file *)
> +    Array.iter (fun file ->
> +      let len = String.length file in
> +      if len >= 4 && String.lowercase (String.sub file (len-4) 4) = ".ovf" then
> +          ovf := file
> +      else if len >= 3 && String.lowercase (String.sub file (len-3) 3) = ".mf" then
> +        mf := file
> +    ) files;
> +
> +    (* verify sha1 from manifest file *)
> +    let mf = dir // !mf in
> +    let rex = Str.regexp "SHA1(\\(.*\\))= *\\(.*?\\).*$" in
> +    let lines = read_file mf in
> +    List.iter (
> +      fun line -> 
> +        if Str.string_match rex line 0 then
> +          let file = Str.matched_group 1 line in
> +          let sha1 = Str.matched_group 2 line in
> +
> +          let cmd = sprintf "sha1sum %s" (dir // file) in

Quoting needed, so:

      let cmd = sprintf "sha1sum %s" (quote (dir // file)) in

> +          let out = external_command ~prog cmd in
> +          if List.exists (fun line -> string_contains line sha1) out == false then

You wouldn't normally write `== false'.  This is more natural and
shorter:

       if not (List.exists (fun line -> string_contains line sha1) out) then

> +            error (f_"Checksum of %s does not match manifes sha1 %s") (file) (sha1)

Don't need parens around parameters `file' and `sha1'.

(f_ ...) is a special case because it's calling a function called
`f_', ie. the gettext function.  Therefore the parser could not
work out if you are calling:

  error f_ "Checksum of ..."

(error + 2 parameters) unless you put in the parentheses:

  error (f_"Checksum of ...")

(1 parameter which is the result of calling function f_).

>  type overlay = {
> -  ov_overlay : string;       (** Local overlay file (qcow2 format). *)
> +  ov_overlay : string;       (** Local overlgy file (qcow2 format). *)

Typo added.

> +let string_contains s1 s2 =

There's a function already defined for this in mllib/common_utils.ml.
`string_find'.  It returns -1 if not found, or >= 0 if found.

> +let read_file filename =

See mllib/common_utils.ml `read_whole_file'.

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-df lists disk usage of guests without needing to install any
software inside the virtual machine.  Supports Linux and Windows.
http://people.redhat.com/~rjones/virt-df/