[Libguestfs] [PATCH v11 3/6] builder: add a template parameter to get_index

[Richard W.M. Jones]

On Thu, Oct 05, 2017 at 04:58:27PM +0200, Cédric Bosdonnat wrote:
> get_index now gets a new template parameter. Setting it to true will
> make the index parsing less picky about missing important data. This
> can be used to parse a partial index file.
> ---
>  builder/builder.ml       |  2 +-
>  builder/index_parser.ml  | 26 ++++++++++++++++++--------
>  builder/index_parser.mli |  5 ++++-
>  3 files changed, 23 insertions(+), 10 deletions(-)
> 
> diff --git a/builder/builder.ml b/builder/builder.ml
> index 3d0dbe7a8..a19eb2d7b 100644
> --- a/builder/builder.ml
> +++ b/builder/builder.ml
> @@ -208,7 +208,7 @@ let main () =
>                                ~tmpdir in
>            match source.Sources.format with
>            | Sources.FormatNative ->
> -            Index_parser.get_index ~downloader ~sigchecker source
> +            Index_parser.get_index ~downloader ~sigchecker ~template:false source
>            | Sources.FormatSimpleStreams ->
>              Simplestreams_parser.get_index ~downloader ~sigchecker source
>        ) sources
> diff --git a/builder/index_parser.ml b/builder/index_parser.ml
> index d6a4e2e86..6f611a7f5 100644
> --- a/builder/index_parser.ml
> +++ b/builder/index_parser.ml
> @@ -25,7 +25,7 @@ open Utils
>  open Printf
>  open Unix
>  
> -let get_index ~downloader ~sigchecker { Sources.uri; proxy } =
> +let get_index ~downloader ~sigchecker ~template { Sources.uri; proxy } =

I don't think this patch is the right approach for reasons given
below.  Notwithstanding that, I would make template into an optional
argument, defaulting to false:

  let get_index ~downloader ~sigchecker ?(template = false)
                { Sources.uri; proxy } =

Then you can remove any place where you've added ‘~template:false’.

>    let corrupt_file () =
>      error (f_"The index file downloaded from ‘%s’ is corrupt.\nYou need to ask the supplier of this file to fix it and upload a fixed version.") uri
>    in
> @@ -99,8 +99,10 @@ let get_index ~downloader ~sigchecker { Sources.uri; proxy } =
>            let arch =
>              try List.assoc ("arch", None) fields
>              with Not_found ->
> -              eprintf (f_"%s: no ‘arch’ entry for ‘%s’\n") prog n;
> -            corrupt_file () in
> +              if template then "" else (
> +                eprintf (f_"%s: no ‘arch’ entry for ‘%s’\n") prog n;
> +                corrupt_file ()
> +              ) in

This leaves arch = "", which subverts strong typing.

I think what is really needed is for the Index.index type to be
modified (or even split into two) so that you can express templates vs
full index entries safely.

Can you explain a bit more about why the arch field would not be
present in a template?

>            let signature_uri =
>              try Some (make_absolute_uri (List.assoc ("sig", None) fields))
>              with Not_found -> None in
> @@ -112,7 +114,7 @@ let get_index ~downloader ~sigchecker { Sources.uri; proxy } =
>            let revision =
>              try Rev_int (int_of_string (List.assoc ("revision", None) fields))
>              with
> -            | Not_found -> Rev_int 1
> +            | Not_found -> if template then Rev_int 0 else Rev_int 1

Do you really mean that the revision should default to 0 for templates?

>              | Failure _ ->
>                eprintf (f_"%s: cannot parse ‘revision’ field for ‘%s’\n") prog n;
>                corrupt_file () in
> @@ -122,11 +124,19 @@ let get_index ~downloader ~sigchecker { Sources.uri; proxy } =
>              try Int64.of_string (List.assoc ("size", None) fields)
>              with
>              | Not_found ->
> -              eprintf (f_"%s: no ‘size’ field for ‘%s’\n") prog n;
> -              corrupt_file ()
> +              if template then
> +                Int64.zero
> +              else (
> +                eprintf (f_"%s: no ‘size’ field for ‘%s’\n") prog n;
> +                corrupt_file ()
> +              )
>              | Failure _ ->
> -              eprintf (f_"%s: cannot parse ‘size’ field for ‘%s’\n") prog n;
> -              corrupt_file () in
> +              if template then
> +                Int64.zero
> +              else (
> +                eprintf (f_"%s: cannot parse ‘size’ field for ‘%s’\n") prog n;
> +                corrupt_file ()
> +              ) in

Same comment as above - abusing type safety for dubious reasons.

Rich.

>            let compressed_size =
>              try Some (Int64.of_string (List.assoc ("compressed_size", None) fields))
>              with
> diff --git a/builder/index_parser.mli b/builder/index_parser.mli
> index b8d8ddf3d..a93e20825 100644
> --- a/builder/index_parser.mli
> +++ b/builder/index_parser.mli
> @@ -16,4 +16,7 @@
>   * 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA.
>   *)
>  
> -val get_index : downloader:Downloader.t -> sigchecker:Sigchecker.t -> Sources.source -> Index.index
> +val get_index : downloader:Downloader.t -> sigchecker:Sigchecker.t -> template:bool -> Sources.source -> Index.index
> +(** [get_index download sigchecker template source] will parse the source
> +    index file into an index entry list. If the template flag is set to
> +    true, the parser will be less picky about missing values. *)
> -- 
> 2.13.2
> 
> _______________________________________________
> Libguestfs mailing list
> Libguestfs at redhat.com
> https://www.redhat.com/mailman/listinfo/libguestfs

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
libguestfs lets you edit virtual machines.  Supports shell scripting,
bindings from many languages.  http://libguestfs.org