[Libguestfs] [PATCH v11 08/10] daemon: Implement inspection of Linux and other Unix-like operating systems.

[Pino Toscano]

On Monday, 31 July 2017 17:40:57 CEST Richard W.M. Jones wrote:
> +(* Parse a os-release file.
> + *
> + * Only few fields are parsed, falling back to the usual detection if we
> + * cannot read all of them.
> + *
> + * For the format of os-release, see also:
> + * http://www.freedesktop.org/software/systemd/man/os-release.html
> + *)
> +let rec parse_os_release release_file data =
> +  let chroot = Chroot.create ~name:"parse_os_release" () in
> +  let lines =
> +    Chroot.f chroot (
> +      fun () ->
> +        if not (is_small_file release_file) then (
> +          eprintf "%s: not a regular file or too large\n" release_file;
> +          None
> +        )
> +        else
> +          Some (read_whole_file release_file)
> +  ) () in

I see this bit (chroot + read_whole_file) is common among various
functions in this file -- could you please place it in an helper
function?

> +  match lines with
> +  | None -> false
> +  | Some lines ->
> +     let lines = String.nsplit "\n" lines in
> +
> +     List.iter (
> +       fun line ->
> +         let line = String.trim line in
> +         if line = "" || line.[0] = '#' then
> +           ()
> +         else (
> +           let key, value = String.split "=" line in
> +           let value =
> +             let n = String.length value in
> +             if n >= 2 && value.[0] = '"' && value.[n-1] = '"' then
> +               String.sub value 1 (n-2)
> +             else
> +               value in
> +           if key = "ID" then (
> +             let distro = distro_of_os_release_id value in
> +             match distro with
> +             | Some _ as distro -> data.distro <- distro
> +             | None -> ()
> +           )
> +           else if key = "PRETTY_NAME" then
> +             data.product_name <- Some value
> +           else if key = "VERSION_ID" then
> +             parse_version_from_major_minor value data
> +         )
> +       ) lines;
> +
> +     (* If we haven't got all the fields, exit right away. *)
> +     if data.distro = None || data.product_name = None then
> +       false
> +     else (
> +       (* os-release in Debian and CentOS does not provide the full
> +        * version number (VERSION_ID), just the major part of it.  If
> +        * we detect that situation then bail out and use the release
> +        * files instead.
> +        *)
> +       match data with
> +       | { distro = Some (DISTRO_DEBIAN|DISTRO_CENTOS);
> +           version = Some (_, 0) } ->
> +          false
> +       | _ -> true
> +     )

parse_os_release should also reset the version to Some (0, 0) (instead
of None) in case the distro is one of the "rolling" ones (only Void at
the moment) -- see also src/inspect-fs-unix.c, lines 240 and on.

> +(* Ubuntu has /etc/lsb-release containing:
> + *   DISTRIB_ID=Ubuntu                                # Distro
> + *   DISTRIB_RELEASE=10.04                            # Version
> + *   DISTRIB_CODENAME=lucid
> + *   DISTRIB_DESCRIPTION="Ubuntu 10.04.1 LTS"         # Product name
> + *
> + * [Ubuntu-derived ...] Linux Mint was found to have this:
> + *   DISTRIB_ID=LinuxMint
> + *   DISTRIB_RELEASE=10
> + *   DISTRIB_CODENAME=julia
> + *   DISTRIB_DESCRIPTION="Linux Mint 10 Julia"
> + * Linux Mint also has /etc/linuxmint/info with more information,
> + * but we can use the LSB file.
> + *
> + * Mandriva has:
> + *   LSB_VERSION=lsb-4.0-amd64:lsb-4.0-noarch
> + *   DISTRIB_ID=MandrivaLinux
> + *   DISTRIB_RELEASE=2010.1
> + *   DISTRIB_CODENAME=Henry_Farman
> + *   DISTRIB_DESCRIPTION="Mandriva Linux 2010.1"
> + * Mandriva also has a normal release file called /etc/mandriva-release.
> + *
> + * CoreOS has a /etc/lsb-release link to /usr/share/coreos/lsb-release containing:
> + *   DISTRIB_ID=CoreOS
> + *   DISTRIB_RELEASE=647.0.0
> + *   DISTRIB_CODENAME="Red Dog"
> + *   DISTRIB_DESCRIPTION="CoreOS 647.0.0"
> + *)
> +and parse_lsb_release release_file data =

TBH, since both parse_os_release and parse_lsb_release are rewritten
in OCaml, I'd try a better approach for them: add an helper function
that read such kind of files (ignoring empty lines, and those starting
with '#'), split them in lines, and map the lines into (key, value)
pairs, also un-quoting the value.  It would not map 1:1 the C
implementations, but in OCaml it would be so much easier than in C
(and that is basically what I'd have liked to do in the first place
when implementing parse_os_release, but it'd have been so much memory
waste).

> +let rec check_linux_root mountable data =
> +  let os_type = OS_TYPE_LINUX in
> +  data.os_type <- Some os_type;
> +
> +  let rec loop = function
> +    | (release_file, parse_fun) :: tests ->
> +       if verbose () then
> +         eprintf "check_linux_root: checking %s\n%!" release_file;
> +       if Is.is_file ~followsymlinks:true release_file then (
> +         if parse_fun release_file data then () (* true => finished *)
> +         else loop tests
> +       ) else loop tests
> +    | [] -> ()
> +  in
> +  loop linux_root_tests;

Would it be possible to move the above loop in an own helper function,
so the check_$OS functions of other OSes can have their own counterpart
of linux_root_tests?  It is true that we don't add new identifications
often, but since it's already implemented here...

> +and check_fstab_entry md_map root_mountable os_type aug entry =
> +  if verbose () then
> +    eprintf "check_fstab_entry: augeas path: %s\n%!" entry;
> +
> +  let is_bsd =
> +    os_type = OS_TYPE_FREEBSD ||
> +    os_type = OS_TYPE_NETBSD ||
> +    os_type = OS_TYPE_OPENBSD in

'match' here?

-- 
Pino Toscano
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: This is a digitally signed message part.
URL: <http://listman.redhat.com/archives/libguestfs/attachments/20170808/1f8352b9/attachment.sig>