6:12 a.m.
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