[Libguestfs] [PATCH 4/5] mllib: add libosinfo DB reading helpers

[Pino Toscano]

On Tuesday, 3 January 2017 11:18:55 CET Cédric Bosdonnat wrote:
> There is already a libosinfo reading function located in src folder to
> get the iso informations. Provide a similar but more generic function
> to be used in ocaml tools.
> ---

I'd rather avoid duplicating the logic used for reading the osinfo-db,
especially that
a) it was touched recently to support the newer osinfo-db, the newer
   internal libosinfo-db, and the old internal libosinfo-db
b) we might support more from the osinfo-db spec in the future (e.g.
   OSINFO_LOCAL_DIR and OSINFO_USER_DIR), especially since they are
   declared "MUST"

A way to do this could be the following (at least IMHO):
1) adapt the current C code (src/osinfo.c) to use a callback (with
   void *opaque) to parse each XML file -- in short, adding a new
   parameter to read_osinfo_db_xml
2) separate osinfo.c in two parts:
  2.a) what reads the osinfo-db and invokes a callback
  2.b) the libguestfs DB (osinfo_db, osinfo_db_size) with
       read_osinfo_db_xml as callback to fill it
3) expose (2.a) to OCaml, with an implementation similar to what Rich
   did recently for the visit API -- see cat/visit.c, mllib/visit-c.c,
   and mllib/visit.ml
4) build & use (2.b) + (3) in virt-builder

This way we would have just the OCaml callback to read the XML file,
and just collecting the list of short-id's.

> +            fun os_file ->
> +              let file_path = distro_path // os_file in
> +              let xml = read_whole_file file_path in
> +              let doc = Xml.parse_memory xml in

Hmm I've seen this pattern already, and IMHO it's time to change it:
the Xml module needs also a parse_file method to parse an existing
file, instead of keeping it all in memory.  Other users of this new
parse_file are in v2v/input_ova.ml, and
v2v/test-harness/v2v_test_harness.ml.

> +let osinfo_read_db filter =
> +  let path = try
> +    Sys.getenv "OSINFO_SYSTEM_DIR"
> +  with Not_found -> "/usr/share/osinfo" in
> +
> +  try osinfo_db_read_three_levels (path // "os") filter with
> +  | _ ->
> +    try osinfo_db_read_three_levels (osinfopath // "os") filter with
> +    | _ ->
> +      try osinfo_db_read_flat (osinfopath // "oses") filter with
> +      | _ -> []

Note that this chain of try..with is too "coarse", and swallows also
errors which needs to be considered fatal (I/O errors, XML issues).

> diff --git a/mllib/osinfo.mli b/mllib/osinfo.mli
> new file mode 100644
> index 000000000..4b881408e
> --- /dev/null
> +++ b/mllib/osinfo.mli
> @@ -0,0 +1,21 @@
> +(* virt-builder
> + * Copyright (C) 2016 - SUSE Inc.
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License along
> + * with this program; if not, write to the Free Software Foundation, Inc.,
> + * 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA.
> + *)
> +
> +val osinfo_read_db : (Xml.xpathctx -> 'a) -> 'a list
> +(** [osinfo_read_db filter] runs the [filter] function on the
> +    XPath context of every osinfo os description XML file. *)

> diff --git a/mllib/osinfopath.ml b/mllib/osinfopath.ml
> new file mode 100644
> index 000000000..a58b8d60f
> --- /dev/null
> +++ b/mllib/osinfopath.ml
> @@ -0,0 +1 @@
> +let osinfopath = "/usr/local/share/libosinfo/db"

Note that a generated file should not be in the patch, and needs to be
added to the top level .gitignore.

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/20170104/207f7173/attachment.sig>