On Tuesday, 3 January 2017 11:18:56 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. ---
Lots of code to review, so this is not an exhaustive review, neither in the style nor in what is actually done; for some coding style issues, I'm usually noting only one occurrence, so please check them in all the sources.
builder/builder_repository.ml | 493 ++++++++++++++++++++++++++++++++++++
I'd call this source as repository_main.ml, similar to what was done for virt-customize (although the reason was different), so there's a pattern when the main source for an OCaml tool is not called "main.ml" or "$tool.ml".
diff --git a/builder/Makefile.am b/builder/Makefile.am index b1ccd49f3..d9f203381 100644 --- a/builder/Makefile.am +++ b/builder/Makefile.am
-bin_PROGRAMS += virt-builder +bin_PROGRAMS += virt-builder virt-builder-repository
You should add virt-builder-repository once, either here or below.
+# virt-builder repository creation tool + +bin_PROGRAMS += virt-builder-repository + +virt_builder_repository_SOURCES = $(REPOSITORY_SOURCES_C) +virt_builder_repository_CPPFLAGS = \ + -I. \ + -I$(top_builddir) \ + -I$(top_srcdir)/gnulib/lib -I$(top_builddir)/gnulib/lib \ + -I$(shell $(OCAMLC) -where) \ + -I$(top_srcdir)/gnulib/lib \ + -I$(top_srcdir)/src \ + -I$(top_srcdir)/fish +virt_builder_repository_CFLAGS = \ + -pthread \ + $(WARN_CFLAGS) $(WERROR_CFLAGS) \ + -Wno-unused-macros \ + $(LIBLZMA_CFLAGS) \ + $(LIBTINFO_CFLAGS) \ + $(LIBXML2_CFLAGS) \ + $(YAJL_CFLAGS) +REPOSITORY_BOBJECTS = $(REPOSITORY_SOURCES_ML:.ml=.cmo) +REPOSITORY_XOBJECTS = $(REPOSITORY_BOBJECTS:.cmo=.cmx) + + +if !HAVE_OCAMLOPT +REPOSITORY_OBJECTS = $(REPOSITORY_BOBJECTS) +else +REPOSITORY_OBJECTS = $(REPOSITORY_XOBJECTS) +endif + +virt_builder_repository_DEPENDENCIES = \ + $(REPOSITORY_OBJECTS) \ + ../mllib/mllib.$(MLARCHIVE) \ + ../customize/customize.$(MLARCHIVE) \ + $(top_srcdir)/ocaml-link.sh +virt_builder_repository_LINK = \ + $(top_srcdir)/ocaml-link.sh -cclib '$(OCAMLCLIBS)' -- \ + $(OCAMLFIND) $(BEST) $(OCAMLFLAGS) $(OCAMLPACKAGES) $(OCAMLLINKFLAGS) \ + $(REPOSITORY_OBJECTS) -o $@
I'd move the whole block quoted above after the same of virt-builder, ...
+man_MANS += virt-builder-repository.1 +noinst_DATA += $(top_builddir)/website/virt-builder-repository.1.html + +virt-builder-repository.1 $(top_builddir)/website/virt-builder-repository.1.html: stamp-virt-builder-repository.pod + +stamp-virt-builder-repository.pod: virt-builder-repository.pod + $(PODWRAPPER) \ + --man virt-builder-repository.1 \ + --html $(top_builddir)/website/virt-builder-repository.1.html \ + --license GPLv2+ \ + --warning safe \ + $< + touch $@
... and the same for this one, after the block of the documentation of virt-builder.
diff --git a/builder/builder_repository.ml b/builder/builder_repository.ml new file mode 100644 index 000000000..682bc9405 --- /dev/null +++ b/builder/builder_repository.ml @@ -0,0 +1,493 @@ +let () = Random.self_init ()
Most probably this is not needed, since this tool will not need to get random bytes.
+type cmdline = { + gpg : string; + gpgkey : string;
Make this a string option ref, so it's clearer to distinguish whether a key was actually specified.
+ [ L"gpg-key" ], Getopt.Set_string ("gpgkey", gpgkey), s_"ID of the GPG key to sign the repo with";
I'd add a short alias, -K.
+ | _ -> + error (f_"too many parameters, at most one '/path/to/repo' is allowed")
"too many parameters, only one path to repository is allowed"
+let make_relative_path base absolute = + if Filename.is_relative absolute then + absolute + else + let expr = sprintf "^%s/\\(.+\\)$" (Str.quote base) in + if Str.string_match (Str.regexp expr) absolute 0 then + Str.matched_group 1 absolute + else + absolute
Hm the path in the "else" branch seems overkill; also, in one of the patches by Tomáš [1] there's one small function, subfolder, which seems to do the same job, so you could polish and add it to Common_utils first (with unit tests, of course ;) ). [1] https://www.redhat.com/archives/libguestfs/2016-December/msg00156.html
+let osinfo_get_short_ids () = + let get_ids xpathctx = + xpath_string_default xpathctx "/libosinfo/os/short-id" "" in + + let ids = Osinfo.osinfo_read_db get_ids in + List.filter (fun id -> id <> "") ids
The last line can be simplified: List.filter ((<>) "") ids Also, the function is ok (modulo changes proposed in patch #4), but it is called every time an osinfo short-id is requested... meaning re-reading fully the whole osinfo-db. Please add a cache for this, reading the osinfo-db only once at the first access. Furthermore, since it's just a collection of strings, I'd use a StringSet for the cache, so the lookup is fast.
+ (* Check that the paths are existing *) + if not (Sys.file_exists (cmdline.repo)) then
No need for round brackets around cmdline.repo.
+ (* Create a temporary folder to work in *) + let tmpdir = Mkdtemp.temp_dir ~base_dir:cmdline.repo "virt-builder-repository." "" in
Too long line, please wrap it.
+ let sigchecker = Sigchecker.create ~gpg:cmdline.gpg + ~check_signature:false + ~gpgkey:No_Key + ~tmpdir:tmpdir in
When the variable for a labelled argument is named exactly like the labelled argument, it can be simplified: ~foo:foo -> ~foo.
+ let index = + try + let index_filename = + List.find ( + fun filename -> Sys.file_exists (cmdline.repo // filename)
Since it is done every iteration (and also below), cmdline.repo // filename could be saved in a variable at the beginning in this try block.
+ let downloader = Downloader.create ~curl:"curl" ~cache:None ~tmpdir:tmpdir in
Maybe the path to curl could be made configurable like in virt-builder.
+ let source = { Sources.name = "input"; + uri = (cmdline.repo // index_filename);
Extra round brackets.
+ (* Check for index/interactive consistency *) + if not cmdline.interactive && index == [] then + error (f_"The repository needs to contain an index file when running in automated mode");
Messages for error should not start as capitalized sentence.
+ let is_supported_format file = + String.is_suffix file "qcow2" || + String.is_suffix file "raw" || + String.is_suffix file "img" in
This function could be in the same block of 'let images' below, as it is not useful outside. Also, Filename.check_suffix could fit better here. Another note is that I've seen filenames for disk images with no extension, e.g. "SomeDisk" or "instance-hda". Maybe we could pick also those files, or try to detect files with unknown extension: the problem is that there is no simple way IIRC to detect RAW images as such.
+ let images = + let files = Array.to_list (Sys.readdir cmdline.repo) in + List.filter (fun f -> is_supported_format f) files in
The last line can be simplified: List.filter is_supported files
+ if r <> 0 then + error (f_"cp command failed copying '%s'") file;
I guess "copying" can be left out from the error message, since mentioning cp implies that already, sort of :)
+ let process_image filename repo index interactive = (
No need for round brackets around whole functions, there is the "in" at the end already. Also, I'd move this big function as top-level in the file, making the "main" easier to read.
+ printf "Preparing %s...\n" filename; + + let filepath = repo // filename in + let qemuimg_cmd = "qemu-img info --output json " ^ (quote filepath) in + let lines = external_command qemuimg_cmd in + let line = String.concat "" lines in
I'd concat the lines with \n, because IIRC the error messages of the yajl library include the line number information.
+ let infos = yajl_tree_parse line in + let format = object_get_string "format" infos in + let size = object_get_number "virtual-size" infos in
The code for getting size and format of a disk image fits as separate helper function IMHO.
+ let xz_path = compress_to filepath cmdline.repo in + let checksum = Checksums.get_checksum (Checksums.SHA512 "") xz_path in + let compressed_size = (Unix.stat xz_path).Unix.st_size in + + let ask message = ( + printf (f_ message); + let value = read_line () in + match value with + | "" -> None + | s -> Some s + ) in + + let rec ask_id () = ( + printf (f_"Identifier: ");
Small hint: when ask details about an image, either put the basename of the file (i.e. "Identifier (foo.qcow2): ") or use the "info" functions to print the progress (like the other tools do).
+ if not (Str.string_match (Str.regexp "[a-zA-Z0-9-_.]+") id 0) then ( + printf (f_"Allowed characters are letters, digits, - _ and .\n");
Such messages should use 'warning' IMHO.
+ let ask_arch () = ( + printf (f_"Architecture. Choose one from the list below:\n"); + let arches = ["x86_64"; "aarch64"; "armv7l"; "i686"; "ppc64"; "ppc64le"; "s390x" ] in + iteri ( + fun i arch -> printf " [%d] %s\n" (i + 1) arch + ) arches; + + let i = ref 0 in + let n = List.length arches in + while !i < 1 || !i > n do + let input = read_line () in + if input = "exit" || input = "q" || input = "quit" then + exit 0 + else + try i := int_of_string input + with Failure _ -> () + done; + List.nth arches (!i - 1) + ) in
While giving a choice of the common options is a good thing, I'd still accept arbitrary architectures, as there is no limitation in other parts of libguestfs. If the input cannot parsed as integer, use the input string as-in.
+ let osinfo_exists = List.exists (fun id -> osinfo = id) ids in
List.exists ((=) osinfo) ids
+ with + | Failure _ -> ("", {Index.printable_name = None;
Hm weird indentation, I'd indent it like: with | Failure -> let entry = { Index.foo = ...; ... } in ( "", entry }
+ let printable_name = + if printable_name = None && interactive then + ask "Display name: "
Missing translation for this string.
+ let arch = + if arch = "" && interactive then + ask_arch () + else ( + if arch = "" then + error (f_"Missing architecture"); + arch + ) in
Hmm convoluted conditions, I'd make it simplier: let arch = if arch = "" then ( if interactive then ask_arch () else error (f_"missing architecture for %s") short_id ) else arch in
+ (id, {Index.printable_name = printable_name;
Space between '{' and Index.etc.
+ osinfo = osinfo; + file_uri = Filename.basename xz_path; + arch = arch; + signature_uri = None; + checksums = Some [(Checksums.SHA512 checksum)]; + revision = revision; + format = Some format; + size = size; + compressed_size = Some (Int64.of_int compressed_size); + expand = expand; + lvexpand = lvexpand; + notes = notes; + hidden = hidden; + aliases = aliases; + sigchecker = sigchecker; + proxy = Curl.UnsetProxy})
Ditto.
+ (* Generate entries for uncompressed images *) + let written_ids = + List.map ( + fun filename -> + let (id, new_entry) = process_image filename cmdline.repo + index cmdline.interactive in
No need for round brackets, as the return value is automatically unwrapped.
+ + Index.print_entry index_channel (id, new_entry);
Note print_entry is designed mostly as debugging helper, not as way to recreate the native index again -- for example, text lines in notes are not indented. I'd add a new function in Index_parser to do this specifically (we can always rename the module to something more fitting later on).
+ output_string index_channel "\n"; + id + ) images in + + (* Write the unchanged entries *) + List.iter ( + fun (id, {Index.printable_name = printable_name; + osinfo = osinfo; + file_uri = file_uri; + arch = arch; + signature_uri = signature_uri; + checksums = checksums; + revision = revision; + format = format; + size = size; + compressed_size = compressed_size; + expand = expand; + lvexpand = lvexpand; + notes = notes; + hidden = hidden; + aliases = aliases; + sigchecker = sigchecker; + proxy = proxy}) ->
No need to extract all the struct values to create a new struct just for changing one or more values -- use the "with" keyword: let entry = { ... } in let new = { entry with size = 10; ... } in
+ (* Remove the index file since we have the signed version of it *) + Sys.remove (tmprepo // "index")
Small TODO item: command line option to not remove the unsigned index when signing -- useful to keep the unsigned index in VCS/etc, while publishing only the signed one.
+ let cmd = [ "mv"; filepath; + (filepath ^ ".bak") ] in + if run_command cmd <> 0 then + error (f_"Failed to create %s backup copy") filename
I'd use a simple helper function, do_mv, for this and the mv below, similar to do_cp in dib/utils.ml.
+ List.iter ( + fun filename -> + let cmd = [ "mv"; tmprepo // filename; + cmdline.repo ] in + if run_command cmd <> 0 then + error (f_"Failed to move %s in repository") (tmprepo // filename) + ) (Array.to_list (Sys.readdir tmprepo));
No need to convert the Array returned by Sys.readdir to List to iterate it, you can iterate just fine on an Array.
diff --git a/builder/virt-builder-repository.pod b/builder/virt-builder-repository.pod new file mode 100644 index 000000000..29d86b4ac --- /dev/null +++ b/builder/virt-builder-repository.pod + [template_id] + name=template display name + file=template_filename.qcow.xz + arch=x86_64 + revision=0 + size=0
Hm, this makes me think Index_parser.get_index could have a "template" parameter and not abort for fields missing that are considered mandatory when parsing a real index.
+The file value needs to match the image name extended with the ".xz" +suffix. Other optional data can be prefilled, for more informations, +see the I<Creating and signing the index file> section in +L<virt-builder(1)> man page.
Use L<virt-builder(1)/Creating and signing the index file>, so it creates the proper linking in HTML. Thanks, -- Pino Toscano