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