VDI I/O error while import large size VDI (> 1TB)

[minli1]

While importing large size VDI (>1TB), error will happen due to an overflow for getting virtual address through BAT. The fix will use integers to convert signed one to unsigned. Signed-off-by: Min Li min.li1@citrix.com

[minli1]

CI failed because of integers library missing in the CI env, build and test passed in local planex env.

[samoht]

@minli1 you can probably fix the CI by add integers in the opam file too.

[minli1]

Thanks for the advice. @samoht You mean https://github.com/mirage/ocaml-vhd/blob/master/vhd-format.opam#L14 ? I thought it should install dependencies while necessary. :)

[minli1]

May I ask help on reviewing this, thanks. @gaborigloi

[robhoes]

@minli1 The CI still complains.

[gaborigloi]

Looks like we need to fix the other opam files the same way.

[gaborigloi]

Sadly we don't yet have Lwt 4.0 in xs-opam, so we'll have to use the old opam file there.

[gaborigloi]

@minli1 Looks like we need to fix safe-string errors in that file (compile with make test) - string and bytes are now separate types, see https://github.com/xapi-project/xs-opam/issues/238

This patch should get make test to work:

diff --git a/disk/disk.ml b/disk/disk.ml
index 65356c4..3645710 100644
--- a/disk/disk.ml
+++ b/disk/disk.ml
@@ -22,7 +22,7 @@ let empty = Int64Map.empty

 let sector_size = 512

-let empty_sector = String.make sector_size '\000'
+let empty_sector = Bytes.make sector_size '\000'

 let write t ofs cstr = Int64Map.add ofs cstr t
 let write_string t ofs s =
@@ -33,7 +33,7 @@ let write_string t ofs s =
 let of_file filename =
   let result = ref Int64Map.empty in
   Lwt_unix.openfile filename [ Unix.O_RDONLY ] 0o0 >>= fun fd ->
-  let buf = String.make sector_size '\000' in
+  let buf = Bytes.make sector_size '\000' in
   let i = ref 0L in
   let finished = ref false in
   let%lwt () =
@@ -42,7 +42,7 @@ let of_file filename =
       finished := n <> sector_size;
       if buf <> empty_sector then begin
         let sector = Cstruct.create n in
-        Cstruct.blit_from_string buf 0 sector 0 n;
+        Cstruct.blit_from_bytes buf 0 sector 0 n;
         result := Int64Map.add !i sector !result;
       end;
       i := Int64.add !i 1L;
@@ -57,9 +57,9 @@ let print_ocaml out t =
   Printf.fprintf out "  let t = ref Disk.empty in\n";
   Int64Map.iter
     (fun ofs cstr ->
-      let buf = String.make (Cstruct.len cstr) '\000' in
-      Cstruct.blit_to_string cstr 0 buf 0 (Cstruct.len cstr);
-      Printf.fprintf out "  t := Disk.write_string !t %LdL \"%s\";\n" ofs (String.escaped buf)
+      let buf = Bytes.make (Cstruct.len cstr) '\000' in
+      Cstruct.blit_to_bytes cstr 0 buf 0 (Cstruct.len cstr);
+      Printf.fprintf out "  t := Disk.write_string !t %LdL \"%s\";\n" ofs (buf |> Bytes.to_string |> String.escaped)
     ) t;
   Printf.fprintf out "  !t\n"

[gaborigloi]

@minli1 Now that we're at it, we could also update the Travis config to test it with newer compiler versions, like 4.06.0 (because that's the version we currently use)

[minli1]

Thanks. @gaborigloi It's almost success except for one job failed, What is the reason for External solver failure ? Is it because of the docker image issue?

 ---> 187d8a67efe0
Step 2/11 : WORKDIR /home/opam/opam-repository
 ---> 4bf31626ddb8
Removing intermediate container 169de2e4d230
Step 3/11 : RUN git pull -q origin master
 ---> Running in a1a39258c96c
 ---> 4f4e2892a1c6
Removing intermediate container a1a39258c96c
Step 4/11 : RUN opam remove travis-opam
 ---> Running in e3ec208ee675
[WARNING] External solver failed with inconsistent return value. Request saved
          to "/home/opam/.opam/log/solver-error-1-1.cudf"
[ERROR] External solver failure, please fix your installation and check
        /home/opam/.opam/config and variable $OPAMEXTERNALSOLVER.
        You may also retry with option --use-internal-solver
The command '/bin/sh -c opam remove travis-opam' returned a non-zero code: 66```

[minli1]

Hi, finally the CI passed and I refined the second commit message; will you please help to look again? @robhoes @gaborigloi @jonludlam Thanks so much.

[minli1]

Thanks @gaborigloi Updated and CI passed.

[lindig]

Just one late comment: The problem is here to read unsigned int32 and to turn it into int64. I believe using the integers library is the right way to go. It would not be too difficult to implement it locally and to avoid the dependency but it would involve an implementation in C and the integers library does exactly that, except for more types.

[jonludlam]

Looks good. Thanks @minli1

[lindig]

One more thought: I believe we only need to convert once an unsigned int32 to signed int64. Rather than relying on a library, would this have been enough?

let uint32 i32 =
  let i64 = Int64.of_int32 i32 in
    Int64.(logand i64 0x0000_0000_ffff_ffffL)

This converts to a signed i64 but resets the upper 32bits to zero that are set to one (through sign extension) when the i32 is negative.