[Libguestfs] [PATCH v4 9/9] Add a virt-builder-repository tool

[Pino Toscano]

On Tuesday, 7 March 2017 15:27:05 CET Cédric Bosdonnat wrote:
> virt-builder-repository allows users to easily create or update
> a virt-builder source repository out of disk images. The tool can
> be run in either interactive or automated mode.
> ---
> [...]
> diff --git a/builder/repository_main.ml b/builder/repository_main.ml
> new file mode 100644
> index 000000000..16c221b34
> +let osinfo_get_short_ids () =
> +  match !osinfo_ids with
> +  | Some ids -> ids
> +  | None -> (
> +    let set = ref StringSet.empty in
> +    let g = open_guestfs () in
> +
> +    Osinfo.read_osinfo_db g#ocaml_handle (
> +      fun filepath ->
> +        let doc = Xml.parse_file filepath in
> +        let xpathctx = Xml.xpath_new_context doc in
> +        let id = xpath_string_default xpathctx "/libosinfo/os/short-id" "" in

Now that I look at this again: the above expression will just read only
one short-id from each XML file, while it can be repeated more than once
(for example the Ubuntu ones have two each, "ubuntuXX.YY" and
"ubuntuCODENAME").

> +let process_image filename repo tmprepo index interactive sigchecker =
> +  message (f_"Preparing %s") filename;
> +
> +  let filepath = repo // filename in
> +  let { format = format; size = size } = get_disk_image_info filepath in
> +  let xz_path = compress_to filepath tmprepo in
> +  let checksum = Checksums.compute_checksum "sha512" xz_path in
> +  let compressed_size = (Unix.stat xz_path).Unix.st_size in

Since later on this is turned into a int64, then most probably you
could use Unix.LargeFile directly.

> +  debug " + %s\n" (String.concat "\n + " images);

This could be:

  info (f_"Found new images: %s") (String.concat " " images);

> +  (* Generate entries for uncompressed images *)
> +  let written_ids =
> +  List.map (
> +    fun filename ->
> +      let id, new_entry = process_image filename cmdline.repo tmprepo
> +                                        index cmdline.interactive sigchecker in
> +
> +      Index_parser.write_entry index_channel (id, new_entry);
> +      id
> +  ) images in

This block is not correctly indented.

> +  (* Write the unchanged entries *)
> +  List.iter (
> +    fun (id, entry) ->
> +      let written = List.mem id written_ids in

Note this will exclude an entry in the existing index, if a new image
with the same identifier but a different architecture is found; see
selected_cli_item in builder.ml.

> +      if not written then
> +        let { Index.file_uri = file_uri } = entry in
> +        if Sys.file_exists file_uri then (
> +          let rel_path =
> +          try
> +            subdirectory cmdline.repo file_uri
> +          with
> +          | Invalid_argument _ ->
> +            file_uri in

This block is not correctly indented.

> +          debug "adding unchanged entry: %s" rel_path;
> +          let rel_entry = { entry with Index.file_uri = rel_path } in
> +          Index_parser.write_entry index_channel (id, rel_entry);
> +        );
> +  ) index;

While the flow of the code from the comment
  (* Generate entries for uncompressed images *)
above up to this point is clear, IMHO it'd be slightly better (and
slightly more OCaml-ish) to:
1) first just process 'images', mapping that to the (name * index) list
2) filter out from 'index' the entries that are in the result of (1)
3) map the result of (2) with what currently done (i.e. processing
   their file_uri)
4) join (3) + (1)
5) write all the entries of (4) at once

Thanks,
-- 
Pino Toscano
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: This is a digitally signed message part.
URL: <http://listman.redhat.com/archives/libguestfs/attachments/20170309/c108c239/attachment.sig>