Re: [Libguestfs] [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/ _______________________________________________ Libguestfs mailing list Libguestfs(a)redhat.com https://www.redhat.com/mailman/listinfo/libguestfs