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