[Libguestfs] [PATCH v3 10/10] Add a virt-builder-repository tool
Pino Toscano
ptoscano at redhat.com
Fri Feb 17 16:42:51 UTC 2017
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 at 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
-------------- 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/20170217/2860d090/attachment.sig>
More information about the Libguestfs
mailing list