Libguestfs - Oct 2017 - [PATCH 1/3] daemon: add split_key_value_strings helper

Add a simple helper to turn a list of strings into key/value pairs,
splitting by '='.
---
 daemon/utils.ml  | 15 +++++++++++++++
 daemon/utils.mli |  6 ++++++
 2 files changed, 21 insertions(+)

diff --git a/daemon/utils.ml b/daemon/utils.ml
index d87ad75db..fd1681a86 100644
--- a/daemon/utils.ml
+++ b/daemon/utils.ml
@@ -229,3 +229,18 @@ let unix_canonical_path path    let path = String.nsplit
"/" path in
   let path = List.filter ((<>) "") path in
   (if is_absolute then "/" else "") ^ String.concat
"/" path
+
+let split_key_value_strings lines +  let lines = List.filter ((<>)
"") lines in
+  let lines = List.filter (fun s -> s.[0] <> '#') lines in
+  List.map (
+    fun line ->
+      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
+      (key, value)
+  ) lines
diff --git a/daemon/utils.mli b/daemon/utils.mli
index f312bde41..9730d06b5 100644
--- a/daemon/utils.mli
+++ b/daemon/utils.mli
@@ -97,5 +97,11 @@ val unix_canonical_path : string -> string
     The path is modified in place because the result is always
     the same length or shorter than the argument passed. *)
 
+val split_key_value_strings : string list -> (string * string) list
+(** Split the lines by the [=] separator, removing the double quotes
+    in the value if present.  Empty lines, or that start with a comment
+    character [#], are ignored.
+*)
+
 (**/**)
 val get_verbose_flag : unit -> bool
-- 
2.13.6

Make use of split_key_value_strings to slightly simplify the parsing of
mdadm output, and the parsing of os-release, and lsb-release files.
In particular, the parsing of lsb-release is simplified a lot, bringing
it much like the parsing of os-release.
---
 daemon/inspect_fs_unix.ml | 93 +++++++++++++++++++----------------------------
 daemon/md.ml              |  9 ++---
 2 files changed, 40 insertions(+), 62 deletions(-)

diff --git a/daemon/inspect_fs_unix.ml b/daemon/inspect_fs_unix.ml
index 3b57899cc..ba947c30e 100644
--- a/daemon/inspect_fs_unix.ml
+++ b/daemon/inspect_fs_unix.ml
@@ -73,31 +73,20 @@ let rec parse_os_release release_file data    match lines
with
   | None -> false
   | Some lines ->
+     let values = split_key_value_strings 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_os_release_version_id value data
+       fun (key, value) ->
+         if key = "ID" then (
+           let distro = distro_of_os_release_id value in
+           match distro with
+           | Some _ as distro -> data.distro <- distro
+           | None -> ()
          )
-       ) lines;
+         else if key = "PRETTY_NAME" then
+           data.product_name <- Some value
+         else if key = "VERSION_ID" then
+           parse_os_release_version_id value data
+       ) values;
 
      (* If we haven't got all the fields, exit right away. *)
      if data.distro = None || data.product_name = None then
@@ -200,48 +189,40 @@ and parse_lsb_release release_file data        *)
      let ok = ref false in
 
+     let values = split_key_value_strings lines in
      List.iter (
-       fun line ->
+       fun (key, value) ->
          if verbose () then
-           eprintf "parse_lsb_release: parsing: %s\n%!" line;
+           eprintf "parse_lsb_release: parsing: %s=%s\n%!" key value;
 
-         if data.distro = None && line = "DISTRIB_ID=Ubuntu"
then (
-           ok := true;
-           data. distro <- Some DISTRO_UBUNTU
-         )
-         else if data.distro = None && line =
"DISTRIB_ID=LinuxMint" then (
-           ok := true;
-           data.distro <- Some DISTRO_LINUX_MINT
-         )
-         else if data.distro = None && line =
"DISTRIB_ID=\"Mageia\"" then (
-           ok := true;
-           data.distro <- Some DISTRO_MAGEIA
-         )
-         else if data.distro = None && line =
"DISTRIB_ID=CoreOS" then (
-           ok := true;
-           data.distro <- Some DISTRO_COREOS
-         )
-         else if String.is_prefix line "DISTRIB_RELEASE=" then (
-           let line = String.sub line 16 (String.length line - 16) in
-           parse_version_from_major_minor line data
-         )
-         else if String.is_prefix line "DISTRIB_DESCRIPTION=\""
||
-                 String.is_prefix line "DISTRIB_DESCRIPTION='"
then (
-           ok := true;
-           let n = String.length line in
-           let product_name = String.sub line 21 (n-22) in
-           data.product_name <- Some product_name
+         if key = "DISTRIB_ID" then (
+           let distro = distro_of_lsb_release_distrib_id value in
+           match distro with
+           | Some _ as distro ->
+             ok := true;
+             data.distro <- distro
+           | None -> ()
          )
-         else if String.is_prefix line "DISTRIB_DESCRIPTION=" then (
+         else if key = "DISTRIB_RELEASE" then
+           parse_version_from_major_minor value data
+         else if key = "DISTRIB_DESCRIPTION" then (
            ok := true;
-           let n = String.length line in
-           let product_name = String.sub line 20 (n-20) in
-           data.product_name <- Some product_name
+           data.product_name <- Some value
          )
-     ) lines;
+     ) values;
 
      !ok
 
+(* DISTRIB_ID="Ubuntu" => Some DISTRO_UBUNTU *)
+and distro_of_lsb_release_distrib_id = function
+  | "CoreOS" -> Some DISTRO_COREOS
+  | "LinuxMint" -> Some DISTRO_LINUX_MINT
+  | "Mageia" -> Some DISTRO_MAGEIA
+  | "Ubuntu" -> Some DISTRO_UBUNTU
+  | value ->
+     eprintf "lsb-release: unknown DISTRIB_ID=%s\n" value;
+     None
+
 and parse_suse_release release_file data    let chroot = Chroot.create
~name:"parse_suse_release" () in
   let lines = Chroot.f chroot (fun () -> read_small_file release_file) () in
diff --git a/daemon/md.ml b/daemon/md.ml
index 5a40a2d83..68d7ea757 100644
--- a/daemon/md.ml
+++ b/daemon/md.ml
@@ -53,7 +53,6 @@ let md_detail md    (* Split the command output into lines. *)
   let out = String.trim out in
   let lines = String.nsplit "\n" out in
-  let lines = List.filter ((<>) "") lines in
 
   (* Parse the output of mdadm -D --export:
    * MD_LEVEL=raid1
@@ -62,11 +61,9 @@ let md_detail md     *
MD_UUID=cfa81b59:b6cfbd53:3f02085b:58f4a2e1
    * MD_NAME=localhost.localdomain:0
    *)
+  let values = split_key_value_strings lines in
   List.map (
-    fun line ->
-      (* Split the line at the equals sign. *)
-      let key, value = String.split "=" line in
-
+    fun (key, value) ->
       (* Remove the MD_ prefix from the key and translate the
        * remainder to lower case.
        *)
@@ -79,4 +76,4 @@ let md_detail md  
       (* Add the key/value pair to the output. *)
       (key, value)
-  ) lines
+  ) values
-- 
2.13.6

In case os-release does not contain VERSION_ID, and it is not an
exception (like Void Linux), then discard the results got from
os-release.

This matches what the old C code did -- e.g. see
commit 32d19e3289bc259901f77398703f16cf6eabd510, and the changes in
commit 0251c0fcaa4fbb3b42968792996748136700c8d8.
---
 daemon/inspect_fs_unix.ml | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/daemon/inspect_fs_unix.ml b/daemon/inspect_fs_unix.ml
index ba947c30e..8c7b1d83c 100644
--- a/daemon/inspect_fs_unix.ml
+++ b/daemon/inspect_fs_unix.ml
@@ -111,6 +111,10 @@ let rec parse_os_release release_file data           
data.version <- Some (0, 0);
           true
 
+       (* No version detected, so fall back to other ways. *)
+       | { version = None } ->
+          false
+
        | _ -> true
      )
 
-- 
2.13.6

This seems like quite a specific function.  Is there not an Augeas
lens for this file.  It would solve this more elegantly.
> +let split_key_value_strings lines > +  let lines = List.filter
((<>) "") lines in
> +  let lines = List.filter (fun s -> s.[0] <> '#') lines
in
> +  List.map (
> +    fun line ->
> +      let key, value = String.split "=" line in
PCRE?  Is whitespace significant?
> +      let value > +        let n = String.length value in
> +        if n >= 2 && value.[0] = '"' &&
value.[n-1] = '"' then
> +          String.sub value 1 (n-2)
Maybe use shell_unquote from C_utils?

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-top is 'top' for virtual machines.  Tiny program with many
powerful monitoring features, net stats, disk stats, logging, etc.
http://people.redhat.com/~rjones/virt-top

On Mon, Oct 16, 2017 at 04:45:51PM +0200, Pino Toscano
wrote:
> In case os-release does not contain VERSION_ID, and it is not an
> exception (like Void Linux), then discard the results got from
> os-release.
> 
> This matches what the old C code did -- e.g. see
> commit 32d19e3289bc259901f77398703f16cf6eabd510, and the changes in
> commit 0251c0fcaa4fbb3b42968792996748136700c8d8.
> ---
>  daemon/inspect_fs_unix.ml | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/daemon/inspect_fs_unix.ml b/daemon/inspect_fs_unix.ml
> index ba947c30e..8c7b1d83c 100644
> --- a/daemon/inspect_fs_unix.ml
> +++ b/daemon/inspect_fs_unix.ml
> @@ -111,6 +111,10 @@ let rec parse_os_release release_file data >       
data.version <- Some (0, 0);
>            true
>  
> +       (* No version detected, so fall back to other ways. *)
> +       | { version = None } ->
> +          false
> +
>         | _ -> true
>       )
>  
> -- 
> 2.13.6
If this patch is valid on its own, then ACK.

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
Fedora Windows cross-compiler. Compile Windows programs, test, and
build Windows installers. Over 100 libraries supported.
http://fedoraproject.org/wiki/MinGW