[Libguestfs] [PATCH v6 10/10] Add a virt-builder-repository tool
Pino Toscano
ptoscano at redhat.com
Wed Apr 26 13:04:11 UTC 2017
On Wednesday, 12 April 2017 14:33:12 CEST 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.
> ---
Many good progresses, thanks :) So far worked fine in my few attempts
when playing with it -- some notes below.
> +let increment_revision revision =
> + match revision with
> + | Utils.Rev_int n -> Utils.Rev_int (n + 1)
> + | Utils.Rev_string s ->
> + if Str.string_match (Str.regexp "^\\(.*[-._]\\)\\([0-9]+\\)$") s 0 then
> + let prefix = Str.matched_group 1 s in
> + let suffix = int_of_string (Str.matched_group 2 s) in
> + Utils.Rev_string (prefix ^ (string_of_int (suffix + 1)))
> + else
> + Utils.Rev_string (s ^ ".1")
The "top-level match" of this function can be simplified:
let increment_revision = function
| Utils.Rev_int n -> Utils.Rev_int (n + 1)
| Utils.Rev_string s ->
etc...
> +let checksums_get_sha512 checksums =
> + match checksums with
> + | Some csums -> (
> + try
> + List.find (
> + fun c ->
> + match c with
> + | Checksums.SHA512 _ -> true
> + | _ -> false
> + ) csums
> + with
> + | _ -> Checksums.SHA512 ""
> + )
> + | None -> Checksums.SHA512 ""
Ditto, in two places:
let checksums_get_sha512 = function
| Some csums ->
try
List.find (
function
| Checksums.SHA512 _ -> true
| _ -> false
) csums
with
Not_found -> Checksums.SHA512 ""
| None -> Checksums.SHA512 ""
Also, List.find is documented to raise only Not_found, so catch that
exception specifically (to avoid a catch-all statements that could
swallow other exceptions, in case the code is expanded in the future).
> +let compress_to file outdir =
> + let outimg = outdir // (Filename.basename file) ^ ".xz" in
> +
> + info "Compressing ...%!";
> + let cmd = sprintf "cat \"%s\" | xz -f --best --block-size=16777216 - >\"%s\""
> + file outimg in
> + if shell_command cmd <> 0 then
This will be done in a much better way once
https://www.redhat.com/archives/libguestfs/2017-April/msg00097.html
is in.
> +let compute_short_id distro major minor =
> + match distro with
> + | "sles"
> + | "sled" -> sprintf "%s%dsp%d" distro major minor
> + | "fedora" -> sprintf "%s%d" distro major
> + | _ -> sprintf "%s%d.%d" distro major minor
I guess it will product a wrong id for e.g. sles 12.0 ;) Also fedora
also uses the major version only. A fixed version of the above, that
handles more distros, could be:
let compute_short_id distro major minor =
match distro with
| "centos" when major >= 7 ->
sprintf "%s%d.0" distro major
| "debian" when major >= 4 ->
sprintf "%s%d" distro major
| ("fedora"|"mageia") ->
sprintf "%s%d" distro major
| "sles" when major = 0 ->
sprintf "%s%d" distro major
| "sles" ->
sprintf "%s%dsp%d" distro major minor
| "ubuntu" ->
sprintf "%s%d.%02d" distro major minor
| _ (* Any other combination. *) ->
sprintf "%s%d.%d" distro major minor
(Also, note that libguestfs considers both SLES and SLED as "sles" --
maybe we could change that, but surely not as part of this series :) )
> +let process_image acc_entries filename repo tmprepo index interactive
> + no_compression sigchecker =
> + message (f_"Preparing %s") filename;
> +
> + let filepath = repo // filename in
> + let { format = format; size = size } = get_disk_image_info filepath in
> + let out_path = if no_compression then
> + filepath
> + else
> + compress_to filepath tmprepo in
A bit confusing indentation for this block -- better something like:
let out_path =
if no_compression then filepath
else compress_to filepath tmprepo in
> + let out_filename = Filename.basename out_path in
> + let checksum = Checksums.compute_checksum "sha512" out_path in
> + let compressed_size = (Unix.LargeFile.stat out_path).Unix.LargeFile.st_size in
> +
> + let ask ?default ?values message =
> + let default_str = match default with
> + | None -> ""
> + | Some x -> sprintf " [%s] " x in
> +
> + let list_str = match values with
> + | None -> ""
> + | Some x ->
> + sprintf (f_"Choose one from the list below:\n %s\n")
> + (String.concat "\n " x) in
> +
> + printf "%s%s%s" message default_str list_str;
> +
> + let value = read_line () in
> + match value with
> + | "" -> default
> + | s -> Some s
I'd map also "-" -> None, otherwise it is impossible to choose to not set
a value for a field when a default is provided. Note this needs custom
handling in ask_id, since an ID is required in interactive mode.
> + let ask_osinfo default =
> + match ask (s_ "osinfo short ID (can be found with osinfo-query os command): ")
> + ~default with
I'd maybe drop the part in parenthesis, and leave the longer
description/hints in the documentation.
> + let arch =
> + if arch = "" then (
> + if interactive then ask_arch inspected_arch
> + else inspected_arch;
> + ) else arch in
> +
> + if arch = "" then
> + error (f_"missing architecture for %s") id;
Maybe ask_arch could take care of keep asking for the architecture
until a valid is give, like ask_id does.
> + let images =
> + let is_supported_format file =
> + let extension = last_part_of file '.' in
> + match extension with
> + | Some ext -> List.mem ext [ "qcow2"; "raw"; "img" ]
> + | None -> file <> "index" in
> + let is_new file =
> + try
> + let _, { Index.checksums = checksums } =
> + List.find (
> + fun (_, { Index.file_uri = file_uri }) ->
> + Filename.basename file_uri = file
> + ) index in
> + let checksum = checksums_get_sha512 checksums in
> + let path = cmdline.repo // file in
> + let file_checksum = Checksums.compute_checksum "sha512" path in
> + checksum <> file_checksum
> + with Not_found -> true in
> + let files = Array.to_list (Sys.readdir cmdline.repo) in
Please filter 'files' here to ignore anything which is not a regular
file (eg directories).
> + (* GPG sign the generated index *)
> + (match cmdline.gpgkey with
> + | None ->
> + debug "Skip index signing"
> + | Some gpgkey ->
> + message (f_"Signing index with GPG key %s") gpgkey;
> + let cmd = sprintf "%s --armor --output %s/index.gpg --export %s"
> + cmdline.gpg tmprepo gpgkey in
Please quote the paths here -- you will need to quote then as whole:
(quote (cmdline.gpg // "index.gpg"))
> + if shell_command cmd <> 0 then
> + error (f_"failed to export GPG key %s") gpgkey;
> +
> + let cmd = sprintf "%s --armor --default-key %s --clearsign %s/index"
> + cmdline.gpg gpgkey tmprepo in
Ditto.
> +=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>
I'd just say "... in the directory specified as argument, ..."
> +folder, compresses the files with a name ending by C<qcow2>, C<raw> or
> +C<img>
Mention also those without extensions are read.
+As in the repository creation use case, a minimal fragment can be
> +added to the index file for the automated mode. This can be done
> +on the signed index even if it may sound a strange idea: the index
> +will be resigned by the tool.
"resign" means something else, I'd use "signed again".
> +=item B<-K> KEYID
> +
> +=item B<--gpg-key> KEYID
> +
> +Specify the GPG key to be used to sign the repository index file.
> +If not provided, the index will left unsigned. C<KEYID> is used to
> +identify the GPG key to use. This value is passed to gpg's
> +C<--default-key> option and can can thus be an email address or a
> +fingerprint.
Double "can".
> +B<NOTE>: by default, virt-builder-repository searches for the key
> +in the user's GPG key ring.
"keyring".
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/20170426/52f3b892/attachment.sig>
More information about the Libguestfs
mailing list