Pino Toscano
2014-Feb-21 16:17 UTC
[Libguestfs] [PATCH] builder: add an arch field to sources read from indexes
Add an architecture field for all the entries in each index, so we know
which architecture they are (not used right now, but will be in the
future).
The problematic part here is properly marking with the correct
architecture: since we only know the current index on libguestfs.org
contains x86_64/amd64 images, entries coming from it are marked that
way; images in all the other indexes (user-provided ones) are not known
to us, so assume they are using the same architecture as virt-builder,
hence the special "@same".
---
builder/builder.ml | 5 +++--
builder/cmdline.ml | 16 +++++++++-------
builder/index_parser.ml | 6 +++++-
builder/index_parser.mli | 3 ++-
4 files changed, 19 insertions(+), 11 deletions(-)
diff --git a/builder/builder.ml b/builder/builder.ml
index 80ccef7..d6d7570 100644
--- a/builder/builder.ml
+++ b/builder/builder.ml
@@ -42,7 +42,7 @@ let main () edit, firstboot, run, format, gpg, hostname,
install, list_format, links,
memsize, mkdirs,
network, output, password_crypto, quiet, root_password, scrub,
- scrub_logfile, selinux_relabel, size, smp, sources, sync, timezone,
+ scrub_logfile, selinux_relabel, size, smp, sources, indexarch, sync,
timezone,
update, upload, writes parse_cmdline () in
@@ -143,7 +143,8 @@ let main () let sigchecker
Sigchecker.create ~debug ~gpg ~check_signature
~gpgkey:(Sigchecker.Fingerprint fingerprint) in
- Index_parser.get_index ~prog ~debug ~downloader ~sigchecker source
+ Index_parser.get_index ~prog ~debug ~downloader ~sigchecker
+ ~arch:indexarch source
) sources
) in
diff --git a/builder/cmdline.ml b/builder/cmdline.ml
index e9e47ae..8959429 100644
--- a/builder/cmdline.ml
+++ b/builder/cmdline.ml
@@ -31,6 +31,8 @@ open Printf
let prog = Filename.basename Sys.executable_name
let default_source =
"http://libguestfs.org/download/builder/index.asc"
+(* So far images in the "default_source" index were x86_64/amd64. *)
+let default_source_architecture = "x86_64"
let parse_cmdline () let display_version () @@ -408,18 +410,18 @@ read the
man page virt-builder(1).
) in
(* Check source(s) and fingerprint(s), or use environment or default. *)
- let sources + let sources, indexarch let list_split = function
"" -> [] | str -> string_nsplit "," str in
let rec repeat x = function
| 0 -> [] | 1 -> [x]
| n -> x :: repeat x (n-1)
in
- let sources - if sources <> [] then sources
+ let sources, indexarch + if sources <> [] then sources,
"@same"
else (
- try list_split (Sys.getenv "VIRT_BUILDER_SOURCE")
- with Not_found -> [ default_source ]
+ try list_split (Sys.getenv "VIRT_BUILDER_SOURCE"),
"@same"
+ with Not_found -> [ default_source ], default_source_architecture
) in
let fingerprints if fingerprints <> [] then fingerprints
@@ -447,12 +449,12 @@ read the man page virt-builder(1).
assert (nr_sources > 0);
(* Combine the sources and fingerprints into a single list of pairs. *)
- List.combine sources fingerprints in
+ List.combine sources fingerprints, indexarch in
mode, arg,
attach, cache, check_signature, curl, debug, delete, delete_on_failure,
edit, firstboot, run, format, gpg, hostname, install, list_format, links,
memsize, mkdirs,
network, output, password_crypto, quiet, root_password, scrub,
- scrub_logfile, selinux_relabel, size, smp, sources, sync, timezone,
+ scrub_logfile, selinux_relabel, size, smp, sources, indexarch, sync,
timezone,
update, upload, writes
diff --git a/builder/index_parser.ml b/builder/index_parser.ml
index 2d4a642..0b9a1b9 100644
--- a/builder/index_parser.ml
+++ b/builder/index_parser.ml
@@ -37,6 +37,7 @@ and entry = {
lvexpand : string option;
notes : (string * string) list;
hidden : bool;
+ arch : string;
sigchecker : Sigchecker.t;
}
@@ -53,6 +54,7 @@ let print_entry chan (name, { printable_name = printable_name;
expand = expand;
lvexpand = lvexpand;
notes = notes;
+ arch = arch;
hidden = hidden }) let fp fs = fprintf chan fs
in
fp "[%s]\n" name;
@@ -91,6 +93,7 @@ let print_entry chan (name, { printable_name = printable_name;
| None -> ()
| Some lvexpand -> fp "lvexpand=%s\n" lvexpand
);
+ fp "arch=%s\n" arch;
List.iter (
fun (lang, notes) ->
match lang with
@@ -108,7 +111,7 @@ and field = string * string option * string (* key +
subkey + value *)
(* Calls yyparse in the C code. *)
external parse_index : string -> sections =
"virt_builder_parse_index"
-let get_index ~prog ~debug ~downloader ~sigchecker source +let get_index ~prog
~debug ~downloader ~sigchecker ~arch source let corrupt_file () eprintf
(f_"\nThe index file downloaded from '%s' is corrupt.\nYou need to
ask the supplier of this file to fix it and upload a fixed version.\n")
source;
@@ -255,6 +258,7 @@ let get_index ~prog ~debug ~downloader ~sigchecker source
lvexpand = lvexpand;
notes = notes;
hidden = hidden;
+ arch = arch;
sigchecker = sigchecker } in
n, entry
) sections in
diff --git a/builder/index_parser.mli b/builder/index_parser.mli
index 3c679b3..4fe9a8a 100644
--- a/builder/index_parser.mli
+++ b/builder/index_parser.mli
@@ -31,8 +31,9 @@ and entry = {
lvexpand : string option;
notes : (string * string) list;
hidden : bool;
+ arch : string;
sigchecker : Sigchecker.t;
}
-val get_index : prog:string -> debug:bool -> downloader:Downloader.t
-> sigchecker:Sigchecker.t -> string -> index
+val get_index : prog:string -> debug:bool -> downloader:Downloader.t
-> sigchecker:Sigchecker.t -> arch:string -> string -> index
--
1.8.3.1
Richard W.M. Jones
2014-Feb-21 16:29 UTC
Re: [Libguestfs] [PATCH] builder: add an arch field to sources read from indexes
On Fri, Feb 21, 2014 at 05:17:59PM +0100, Pino Toscano wrote:> Add an architecture field for all the entries in each index, so we know > which architecture they are (not used right now, but will be in the > future). > > The problematic part here is properly marking with the correct > architecture: since we only know the current index on libguestfs.org > contains x86_64/amd64 images, entries coming from it are marked that > way; images in all the other indexes (user-provided ones) are not known > to us, so assume they are using the same architecture as virt-builder, > hence the special "@same".A few questions: - Would it be better/easier if we ditched the whole idea of VIRT_BUILDER_SOURCE being a list? It wasn't really a great idea in hindsight, and we could change it now. It's even possible to not have VIRT_BUILDER_SOURCE at all, everything is configured through the config files. - Can we force people to put an arch= field in the index, and if it is missing, assume x86_64? (Both of these would mean we didn't need @same, IIUI.) Rich.> --- > builder/builder.ml | 5 +++-- > builder/cmdline.ml | 16 +++++++++------- > builder/index_parser.ml | 6 +++++- > builder/index_parser.mli | 3 ++- > 4 files changed, 19 insertions(+), 11 deletions(-) > > diff --git a/builder/builder.ml b/builder/builder.ml > index 80ccef7..d6d7570 100644 > --- a/builder/builder.ml > +++ b/builder/builder.ml > @@ -42,7 +42,7 @@ let main () > edit, firstboot, run, format, gpg, hostname, install, list_format, links, > memsize, mkdirs, > network, output, password_crypto, quiet, root_password, scrub, > - scrub_logfile, selinux_relabel, size, smp, sources, sync, timezone, > + scrub_logfile, selinux_relabel, size, smp, sources, indexarch, sync, timezone, > update, upload, writes > parse_cmdline () in > > @@ -143,7 +143,8 @@ let main () > let sigchecker > Sigchecker.create ~debug ~gpg ~check_signature > ~gpgkey:(Sigchecker.Fingerprint fingerprint) in > - Index_parser.get_index ~prog ~debug ~downloader ~sigchecker source > + Index_parser.get_index ~prog ~debug ~downloader ~sigchecker > + ~arch:indexarch source > ) sources > ) in > > diff --git a/builder/cmdline.ml b/builder/cmdline.ml > index e9e47ae..8959429 100644 > --- a/builder/cmdline.ml > +++ b/builder/cmdline.ml > @@ -31,6 +31,8 @@ open Printf > let prog = Filename.basename Sys.executable_name > > let default_source = "http://libguestfs.org/download/builder/index.asc" > +(* So far images in the "default_source" index were x86_64/amd64. *) > +let default_source_architecture = "x86_64" > > let parse_cmdline () > let display_version () > @@ -408,18 +410,18 @@ read the man page virt-builder(1). > ) in > > (* Check source(s) and fingerprint(s), or use environment or default. *) > - let sources > + let sources, indexarch > let list_split = function "" -> [] | str -> string_nsplit "," str in > let rec repeat x = function > | 0 -> [] | 1 -> [x] > | n -> x :: repeat x (n-1) > in > > - let sources > - if sources <> [] then sources > + let sources, indexarch > + if sources <> [] then sources, "@same" > else ( > - try list_split (Sys.getenv "VIRT_BUILDER_SOURCE") > - with Not_found -> [ default_source ] > + try list_split (Sys.getenv "VIRT_BUILDER_SOURCE"), "@same" > + with Not_found -> [ default_source ], default_source_architecture > ) in > let fingerprints > if fingerprints <> [] then fingerprints > @@ -447,12 +449,12 @@ read the man page virt-builder(1). > assert (nr_sources > 0); > > (* Combine the sources and fingerprints into a single list of pairs. *) > - List.combine sources fingerprints in > + List.combine sources fingerprints, indexarch in > > mode, arg, > attach, cache, check_signature, curl, debug, delete, delete_on_failure, > edit, firstboot, run, format, gpg, hostname, install, list_format, links, > memsize, mkdirs, > network, output, password_crypto, quiet, root_password, scrub, > - scrub_logfile, selinux_relabel, size, smp, sources, sync, timezone, > + scrub_logfile, selinux_relabel, size, smp, sources, indexarch, sync, timezone, > update, upload, writes > diff --git a/builder/index_parser.ml b/builder/index_parser.ml > index 2d4a642..0b9a1b9 100644 > --- a/builder/index_parser.ml > +++ b/builder/index_parser.ml > @@ -37,6 +37,7 @@ and entry = { > lvexpand : string option; > notes : (string * string) list; > hidden : bool; > + arch : string; > > sigchecker : Sigchecker.t; > } > @@ -53,6 +54,7 @@ let print_entry chan (name, { printable_name = printable_name; > expand = expand; > lvexpand = lvexpand; > notes = notes; > + arch = arch; > hidden = hidden }) > let fp fs = fprintf chan fs in > fp "[%s]\n" name; > @@ -91,6 +93,7 @@ let print_entry chan (name, { printable_name = printable_name; > | None -> () > | Some lvexpand -> fp "lvexpand=%s\n" lvexpand > ); > + fp "arch=%s\n" arch; > List.iter ( > fun (lang, notes) -> > match lang with > @@ -108,7 +111,7 @@ and field = string * string option * string (* key + subkey + value *) > (* Calls yyparse in the C code. *) > external parse_index : string -> sections = "virt_builder_parse_index" > > -let get_index ~prog ~debug ~downloader ~sigchecker source > +let get_index ~prog ~debug ~downloader ~sigchecker ~arch source > let corrupt_file () > eprintf (f_"\nThe index file downloaded from '%s' is corrupt.\nYou need to ask the supplier of this file to fix it and upload a fixed version.\n") > source; > @@ -255,6 +258,7 @@ let get_index ~prog ~debug ~downloader ~sigchecker source > lvexpand = lvexpand; > notes = notes; > hidden = hidden; > + arch = arch; > sigchecker = sigchecker } in > n, entry > ) sections in > diff --git a/builder/index_parser.mli b/builder/index_parser.mli > index 3c679b3..4fe9a8a 100644 > --- a/builder/index_parser.mli > +++ b/builder/index_parser.mli > @@ -31,8 +31,9 @@ and entry = { > lvexpand : string option; > notes : (string * string) list; > hidden : bool; > + arch : string; > > sigchecker : Sigchecker.t; > } > > -val get_index : prog:string -> debug:bool -> downloader:Downloader.t -> sigchecker:Sigchecker.t -> string -> index > +val get_index : prog:string -> debug:bool -> downloader:Downloader.t -> sigchecker:Sigchecker.t -> arch:string -> string -> index > -- > 1.8.3.1 > > _______________________________________________ > Libguestfs mailing list > Libguestfs@redhat.com > https://www.redhat.com/mailman/listinfo/libguestfs-- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones virt-df lists disk usage of guests without needing to install any software inside the virtual machine. Supports Linux and Windows. http://people.redhat.com/~rjones/virt-df/
Pino Toscano
2014-Feb-21 16:49 UTC
Re: [Libguestfs] [PATCH] builder: add an arch field to sources read from indexes
On Friday 21 February 2014 16:29:23 Richard W.M. Jones wrote:> On Fri, Feb 21, 2014 at 05:17:59PM +0100, Pino Toscano wrote: > > Add an architecture field for all the entries in each index, so we > > know which architecture they are (not used right now, but will be > > in the future). > > > > The problematic part here is properly marking with the correct > > architecture: since we only know the current index on libguestfs.org > > contains x86_64/amd64 images, entries coming from it are marked that > > way; images in all the other indexes (user-provided ones) are not > > known to us, so assume they are using the same architecture as > > virt-builder, hence the special "@same". > > A few questions: > > - Would it be better/easier if we ditched the whole idea of > VIRT_BUILDER_SOURCE being a list? It wasn't really a great idea in > hindsight, and we could change it now. It's even possible to not > have VIRT_BUILDER_SOURCE at all, everything is configured through the > config files.I was following the "keep compatibility" principle we have so far in libguestfs, so part of the current patch was also in that sense. (Of course the additions of arch to Index_parser would still apply.) Ditching VIRT_BUILDER_SOURCE (and VIRT_BUILDER_FINGERPRINT with it) would be okay for me as well, that could indeed simplify some stuff that I'm currently working on.> - Can we force people to put an arch= field in the index, and if it > is missing, assume x86_64?My idea/implementation so far had arch= in the .conf files: this way, one could in principle keep an existing (optionally signed) index somewhere, adding a .conf file pointing to it, and describing which architecture would have the images in that index. Hmm, now that I think about it, - adding arch= in indexes is backward compatible - an user-supplied index could also have images with different architectures so I will move arch= to indexes. Regarding requiring arch= in indexes: this would mean users providing indexes right now would need to update them (and re-sign, in case) to have them working with future virt-builder 1.26. While not a big deal, it would prevent the "ship this .conf file pointing to my index" to work OOTB. Having arch= optional (with implicit "@same") would not be a big deal either, at least IMHO. -- Pino Toscano