10:42 a.m.
On Friday, 10 February 2017 16:06: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.
---
Still not a comprehensive review, but here there are few more notes.
diff --git a/builder/repository_main.ml b/builder/repository_main.ml
new file mode 100644
index 000000000..89807f9d5
--- /dev/null
+++ b/builder/repository_main.ml
@@ -0,0 +1,487 @@
+(* virt-builder
+ * Copyright (C) 2016 SUSE Inc.
2017 too? (ditto for the .pod file)
+type disk_image_infos = {
+ format : string;
+ size : int64;
+}
"disk_image_info", since "information" is singular.
+ {
+ gpg = gpg;
+ gpgkey = gpgkey;
+ interactive = interactive;
+ keep_unsigned = keep_unsigned;
+ repo = repo;
+ }
Indented too much?
+let osinfo_ids = ref None
+
+let osinfo_get_short_ids () =
+ match !osinfo_ids with
+ | Some ids -> ids
+ | None -> (
+ let ids = ref [] 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
+ if id <> "" then
+ ids := id :: !ids
+ );
+ g#close ();
+ let ids_set = StringSet.of_list(!ids) in
+ osinfo_ids := Some ids_set;
+ ids_set
+ )
osinfo_get_short_ids does not need an intermediate list to fill, to
convert as Set at the end of the reading, as a Set can be filled
directly:
let set = ref StringSet.empty in
...
if id <> "" then
set := StringSet.add id !set
...
osinfo_ids := Some (!set)
+(* Move files in tmprepo into the repository *)
+let do_mv src dest =
+ let cmd = [ "mv"; src; dest ] in
+ run_command cmd
This must check the result of run_command.
+let compress_to file outdir =
+ info "Copying image to temporary folder ...%!";
+ let outimg = outdir // (Filename.basename file) in
+ let cmd = [ "cp" ] @
+ (if verbose () then [ "-v" ] else []) @
+ [ file; outimg ] in
+ let r = run_command cmd in
+ if r <> 0 then
+ error (f_"cp command failed '%s'") file;
Most probably the error message could be improved, e.g.:
"copy of %s to %s" file outdir
and added to the do_cp from dib/utils.ml, and the function moved to
Common_utils.
+let get_disk_image_infos filepath =
+ let qemuimg_cmd = "qemu-img info --output json " ^ (quote filepath) in
+ let lines = external_command qemuimg_cmd in
+ let line = String.concat "\n" lines in
+ let infos = yajl_tree_parse line in
+ { format = object_get_string "format" infos;
+ size = object_get_number "virtual-size" infos }
Could this be formatted better, e.g.:
{
format = ...;
size = ...
}
+let process_image filename repo tmprepo index interactive sigchecker
=
+ info "Preparing %s..." filename;
I guess this should be "progress", have no ellipsis, and marked for
translation.
+ let ask message = (
There's no need for ( ... ) in let foo = ... in
+ printf message;
+ let value = read_line () in
+ match value with
+ | "" -> None
+ | s -> Some s
+ ) in
+
+ let rec ask_id () = (
Ditto.
+ printf (f_"Identifier: ");
+ let id = read_line () in
+ if not (Str.string_match (Str.regexp "[a-zA-Z0-9-_.]+") id 0) then (
+ warning (f_"Allowed characters are letters, digits, - _ and .");
+ ask_id ();
+ ) else
+ id;
+ ) in
+
+ let ask_arch () = (
Ditto.
+ 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 arch = ref None in
+ while !arch <> None do
+ let input = read_line () in
+ if input = "exit" || input = "q" || input = "quit"
then
+ exit 0
+ else
+ try
+ let i = int_of_string input in
+ arch := Some (List.nth arches (i - 1))
+ with Failure _ -> arch := Some input
+ done;
+ match !arch with
+ | None -> "" (* Should never happen *)
assert false, then. Also, the while loop seems not executed at all,
since arch is None and the condition checks for <> None?
Also, if the condition would be inverted, the loop would be executed
just once.
+ let ask_osinfo () =
+ printf (f_ "osinfo short ID (can be found with osinfo-query os command):
");
+ let value = read_line () in
+ match value with
+ | "" -> None
+ | osinfo ->
+ let osinfo_ids = osinfo_get_short_ids () in
+ if not (StringSet.mem osinfo osinfo_ids) then
+ warning (f_"'%s' is not a libosinfo OS id") osinfo;
"'%s' is not a recognized osinfo OS id; using it anyway"
+ (* Do we have an entry for that file already? *)
+ let file_entry =
+ try
+ List.hd (
+ List.filter (
+ fun (id, { Index.file_uri=file_uri }) ->
+ (Filename.basename file_uri) = ((Filename.basename filename) ^
".xz")
let xzfn = Filename.basename filename ^ ".xz" in
and declated outside of the List.hd.
+ let id =
+ if id = "" && interactive then
+ ask_id ()
+ else (
+ if id = "" then
+ error (f_"Missing image identifier");
+ id
+ ) in
The double id = "" check makes it a bit confusing; IMHO it can be
simplified as:
let id =
if id = "" then (
if interactive then ask_id ()
else error (f_"missing image identifier");
) else id in
+ (* Check that the paths are existing *)
+ if not (Sys.file_exists cmdline.repo) then
+ error (f_"Repository folder '%s' doesn't exist") cmdline.repo;
Messages for "error" should not start with a capital letter (this
applies to all the occurrencies in this tool, not just this).
+ (* Create a temporary folder to work in *)
+ let tmpdir = Mkdtemp.temp_dir ~base_dir:cmdline.repo
+ "virt-builder-repository." "" in
+ rmdir_on_exit tmpdir;
+
+ let tmprepo = tmpdir // "repo" in
+ Unix.mkdir tmprepo 0o700;
I'd use mkdir_p to ensure all the directories are available; in this
case Unix.mkdir works too, since the parent directory is created by
Mkdtemp.temp_dir, but having it as safety measure won't hurt.
+ let images =
+ let is_supported_format file =
+ let extension = last_part_of file '.' in
+ match extension with
+ | Some ext ->
+ let allowed = StringSet.of_list ["qcow2"; "raw";
"img"] in
+ StringSet.mem ext allowed
A list in this case can be fine, since the number of items is very
small -- so:
List.mem ext [ "qcow2"; "raw"; "img" ]
+ debug " + %s\n" (String.concat "\n + "
images);
List.iter (debug " + %s\n") images;
+ (* Write the unchanged entries *)
+ List.iter (
+ fun (id, entry) ->
+ let written = List.exists (fun written_id -> written_id = id) written_ids in
List.mem?
+ | Some gpgkey ->
+ debug "Signing index with GPG key %s" gpgkey;
This could be a "progress" message.
+ debug "Creating index backup copy";
Ditto.
+ debug "Moving files to final destination";
Ditto.
diff --git a/builder/test-docs.sh b/builder/test-docs.sh
index 83e2f4961..e65fdd3fe 100755
--- a/builder/test-docs.sh
+++ b/builder/test-docs.sh
@@ -23,3 +23,6 @@ $srcdir/../podcheck.pl virt-builder.pod virt-builder \
--insert $srcdir/../customize/customize-synopsis.pod:__CUSTOMIZE_SYNOPSIS__ \
--insert $srcdir/../customize/customize-options.pod:__CUSTOMIZE_OPTIONS__ \
--ignore=--check-signatures,--no-check-signatures
+
+$srcdir/../podcheck.pl virt-builder-repository.pod virt-builder-repository \
+ --ignore=--check-signatures,--no-check-signatures
The --ignore here does not apply, since virt-builder-repository does
not have these options.
diff --git a/builder/virt-builder-repository.pod
b/builder/virt-builder-repository.pod
new file mode 100644
index 000000000..697deddec
--- /dev/null
+++ b/builder/virt-builder-repository.pod
@@ -0,0 +1,183 @@
+=begin html
+
+<img src="virt-builder.svg" width="250"
+ style="float: right; clear: right;" />
+
+=end html
+
+=head1 NAME
+
+virt-builder-repository - Build virt-builder source repository easily
+
+=head1 SYNOPSIS
+
+ virt-builder-repository /path/to/repository
+ [-i|--interactive] [--gpg-key KEYID]
+
+=head1 DESCRIPTION
+
+Virt-builder is a tool for quickly building new virtual machines. It can
+be configured to use template repositories. However creating and
+maintaining a repository involves many tasks which can be automated.
+virt-builder-repository is a tool helping to manage these repositories.
+
+Virt-builder-repository loops over the files in the C</path/to/repository>
+folder, compresses the files with a name ending by C<qcow2>, C<raw> or
+C<img>, extracts data from them and creates or updates the C<index> file.
+
+Some of the image-related data needed for the index file can't be
+computed from the image file. virt-builder-repository first tries to
+find them in the existing index file. If data are still missing after
+this, they are prompted in interactive mode, otherwise an error will
+be triggered.
+
+If a C<KEYID> is provided, the generated index file will be signed
+with this GPG key.
+
+=head1 EXAMPLES
+
+=head2 Create the initial repository
+
+Create a folder and copy the disk image template files in it. Then
+run a command like the following one:
+
+ virt-builder-repository --gpg-key "joe(a)hacker.org" -i /path/to/folder
+
+Note that this example command runs in interactive mode. To run in
+automated mode, a minimal index file needs to be created before running
+the command containing sections like this one:
+
+ [template_id]
+ name=template display name
+ file=template_filename.qcow.xz
+ arch=x86_64
+ revision=0
+ expand=/dev/sda3
+
+The file value needs to match the image name extended with the ".xz"
+suffix. Other optional data can be prefilled, for more informations,
+see L<virt-builder(1)/Creating and signing the index file> man page.
"man page" can be removed, since it will be an HTML link for the HTML
output.
Thanks,
--
Pino Toscano