Libguestfs - Aug 2014 - Re: [PATCH] v2v: adding input -i ova

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/

Added the fixed patch

On 21.08.14 14:04, Richard W.M. Jones wrote:
> 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/
> 
> _______________________________________________
> Libguestfs mailing list
> Libguestfs@redhat.com
> https://www.redhat.com/mailman/listinfo/libguestfs

One comment down..

On 21.08.14 17:09, Shahar Havivi wrote:
> Added the fixed patch
> 
> On 21.08.14 14:04, Richard W.M. Jones wrote:
> > 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/
> > 
> > _______________________________________________
> > Libguestfs mailing list
> > Libguestfs@redhat.com
> > https://www.redhat.com/mailman/listinfo/libguestfs
> From 0efe4755d2c981d55e3f0016ce5d7a1afd275c9a Mon Sep 17 00:00:00 2001
> From: Shahar Havivi <shaharh@redhat.com>
> Date: Thu, 21 Aug 2014 15:54:38 +0300
> Subject: [PATCH] v2v: adding input -i ova
> 
> Signed-off-by: Shahar Havivi <shaharh@redhat.com>
> ---
>  po/POTFILES-ml    |  1 +
>  v2v/Makefile.am   |  2 ++
>  v2v/cmdline.ml    | 14 +++++++--
>  v2v/input_ova.ml  | 91
+++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  v2v/input_ova.mli | 22 ++++++++++++++
>  5 files changed, 128 insertions(+), 2 deletions(-)
>  create mode 100644 v2v/input_ova.ml
>  create mode 100644 v2v/input_ova.mli
> 
> diff --git a/po/POTFILES-ml b/po/POTFILES-ml
> index 2001f47..35a94d3 100644
> --- a/po/POTFILES-ml
> +++ b/po/POTFILES-ml
> @@ -88,6 +88,7 @@ v2v/convert_linux.ml
>  v2v/convert_windows.ml
>  v2v/input_disk.ml
>  v2v/input_libvirt.ml
> +v2v/input_ova.ml
>  v2v/lib_esx.ml
>  v2v/lib_linux.ml
>  v2v/output_RHEV.ml
> diff --git a/v2v/Makefile.am b/v2v/Makefile.am
> index 3eec99a..5f2fa74 100644
> --- a/v2v/Makefile.am
> +++ b/v2v/Makefile.am
> @@ -32,6 +32,7 @@ SOURCES_MLI = \
>  	lib_linux.mli \
>  	input_disk.mli \
>  	input_libvirt.mli \
> +	input_ova.mli \
>  	output_libvirt.mli \
>  	output_local.mli \
>  	output_RHEV.mli \
> @@ -48,6 +49,7 @@ SOURCES_ML = \
>  	lib_linux.ml \
>  	input_disk.ml \
>  	input_libvirt.ml \
> +	input_ova.ml \
>  	convert_linux.ml \
>  	convert_windows.ml \
>  	output_libvirt.ml \
> diff --git a/v2v/cmdline.ml b/v2v/cmdline.ml
> index bec6d51..7842a4f 100644
> --- a/v2v/cmdline.ml
> +++ b/v2v/cmdline.ml
> @@ -54,6 +54,7 @@ let parse_cmdline () >      | "disk" |
"local" -> input_mode := `Disk
>      | "libvirt" -> input_mode := `Libvirt
>      | "libvirtxml" -> input_mode := `LibvirtXML
> +    | "ova" -> input_mode := `OVA
>      | s ->
>        error (f_"unknown -i option: %s") s
>    in
> @@ -105,7 +106,7 @@ let parse_cmdline () >    let argspec = Arg.align [
>      "--bridge",  Arg.String add_bridge,     "in:out "
^ s_"Map bridge 'in' to 'out'";
>      "--debug-gc",Arg.Set debug_gc,          " " ^
s_"Debug GC and memory allocations";
> -    "-i",        Arg.String set_input_mode,
"disk|libvirt|libvirtxml " ^ s_"Set input mode (default:
libvirt)";
> +    "-i",        Arg.String set_input_mode,
"disk|libvirt|libvirtxml|ova " ^ s_"Set input mode (default:
libvirt)";
>      "-ic",       Arg.Set_string input_conn, "uri " ^
s_"Libvirt URI";
>      "-if",       Arg.Set_string input_format,
>                                              "format " ^
s_"Input format (for -i disk)";
> @@ -231,7 +232,16 @@ read the man page virt-v2v(1).
>          | [filename] -> filename
>          | _ ->
>            error (f_"expecting a libvirt XML file name on the command
line") in
> -      Input_libvirt.input_libvirtxml verbose filename in
> +      Input_libvirt.input_libvirtxml verbose filename
> +
> +    | `OVA ->
> +      (* -i ova: Expecting an ova filename (tar file). *)
> +      let filename > +        match args with
> +        | [filename] -> filename
> +        | _ ->
> +          error (f_"expecting an OVA file name on the command
line") in
> +      Input_ova.input_ova verbose filename in
>  
>    (* Parse the output mode. *)
>    let output > diff --git a/v2v/input_ova.ml b/v2v/input_ova.ml
> new file mode 100644
> index 0000000..b36d135
> --- /dev/null
> +++ b/v2v/input_ova.ml
> @@ -0,0 +1,91 @@
> +(* virt-v2v
> + * Copyright (C) 2009-2014 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.
> + *)
> +
> +open Printf
> +
> +open Common_gettext.Gettext
> +open Common_utils
> +
> +open Types
> +open Utils
> +
> +let parse_ovf dir ovf > +  let source = {
> +    s_dom_type = "kvm";
> +    s_name = "";
> +    s_orig_name = "";
> +    s_memory = 0L;
> +    s_vcpu = 1;
> +    s_arch = "";
> +    s_features = [""];
> +    s_display = None;
> +    s_disks = [];
> +    s_removables = [];
> +    s_nics = [];
> +  } in
> +  source
> +
> +class input_ova verbose ova > +object
> +  inherit input verbose
> +
> +  method as_options = "-i ova " ^ ova
> +
> +  method source () > +    (* get ova directory *)
> +    let dir = Filename.dirname (absolute_path ova) in
> +    (* extract ova (tar) file *)
> +    let cmd = sprintf "tar -xf %s -C %s" (quote ova) (quote dir)
in
> +
> +    if Sys.command cmd <> 0 then
> +        error (f_"error running command: %s") cmd;
> +
> +    let files = Sys.readdir dir in
> +    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_whole_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" (quote(dir // file)) in
> +          let out = external_command ~prog cmd in
> +          if not (List.exists (fun line -> string_find line sha1 == -1)
out) then
> +              error (f_"Checksum of %s does not match manifest sha1
%s") file sha1;
> +        else
> +            error (f_"error cant parse mf: %s, line: %s") mf
line;
> +    ) (Str.split (Str.regexp "\n") lines);
Not sure regarding the split if there is a better way...
> +
> +    parse_ovf dir ovf
> +end
> +
> +let input_ova = new input_ova
> diff --git a/v2v/input_ova.mli b/v2v/input_ova.mli
> new file mode 100644
> index 0000000..d4c347e
> --- /dev/null
> +++ b/v2v/input_ova.mli
> @@ -0,0 +1,22 @@
> +(* virt-v2v
> + * Copyright (C) 2009-2014 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.
> + *)
> +
> +(** [-i ova] source. *)
> +
> +val input_ova : bool -> string -> Types.input
> +(** [input_ova filename] sets up an input from vmware ova file. *)
> -- 
> 1.9.3
>

On Thu, Aug 21, 2014 at 05:09:13PM +0300, Shahar Havivi wrote:
[...]

You'll also need to update documentation (v2v/virt-v2v.pod) and maybe
add a test if it is possible to test this without carrying around
huge/proprietary files.
> +    (* verify sha1 from manifest file *)
> +    let mf = dir // !mf in
> +    let rex = Str.regexp "SHA1(\\(.*\\))= *\\(.*?\\).*$" in
> +    let lines = read_whole_file mf in
In here, add:

   let lines = string_nsplit "\n" lines in

and replace the (Str.split ... lines) with just lines (after List.iter).
   
> +    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" (quote(dir // file)) in
Might be easier to use:


       let cmd = sprintf "sha1sum %s | awk '{print $1}'"
                   (quote (dir // file)) in
> +          let out = external_command ~prog cmd in
and then:

   (match out with
   | [] -> error (f_"no output from sha1sum command, see previous
errors")
   | [line] ->
      let len = String.length sha1 in
      let line         if len > 0 && sha1.[len-1] = '\n' then
          String.sub sha1 0 (len-1)
        else line in
      if line <> sha1 then
        error (f_"Checksum of %s does not match manifest sha1 %s")
file sha1;
   | _::_ -> error (f_"cannot parse output of sha1sum command")
   );

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
libguestfs lets you edit virtual machines.  Supports shell scripting,
bindings from many languages.  http://libguestfs.org