Libguestfs - Jul 2022 - [PATCH] Add simple parsing logic for hostname file.

How about this instead?

It actually changes the behaviour slightly.  Currently a /etc/hostname
which contains the single character '\n' would return
inspect_get_hostname = "" (which seems wrong).  This is the default
/etc/hostname for virt-builder images.

I think this is because in the current code, read_small_file actually
returns ?Some [""; ""]?, so it takes the branch where it
matches
?| Some (hostname :: _)? which returns empty string.

Anyway, after this patch it returns "unknown" which is documented as
"the hostname could not be determined", which I think is more correct
behaviour.  I tested it also on files with blank lines and comments
and so on, and it seems to do what is necessary, although it could do
with a proper regression test.

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-builder quickly builds VMs from scratch
http://libguestfs.org/virt-builder.1.html
-------------- next part --------------
>From 3b6897c8ba9c8d438e8599af93188e65cced6ec8 Mon Sep 17 00:00:00 2001
From: "Richard W.M. Jones" <rjones at redhat.com>
Date: Wed, 20 Jul 2022 12:33:09 +0100
Subject: [PATCH] daemon: Parse /etc/hostname files containing comments

Thanks: Dawid Zamirski
Link: https://www.freedesktop.org/software/systemd/man/hostname.html
---
 daemon/inspect_fs_unix.ml | 19 ++++++++++++++++---
 1 file changed, 16 insertions(+), 3 deletions(-)

diff --git a/daemon/inspect_fs_unix.ml b/daemon/inspect_fs_unix.ml
index 63cb279d08..8045ef0d43 100644
--- a/daemon/inspect_fs_unix.ml
+++ b/daemon/inspect_fs_unix.ml
@@ -551,15 +551,28 @@ and check_hostname_linux ()       else
        None
 
-(* Parse the hostname where it is stored directly in a file. *)
+(* Parse the hostname where it is stored directly in a file.
+ *
+ * For /etc/hostname:
+ * "The file should contain a single newline-terminated hostname
+ * string. Comments (lines starting with a "#") are ignored."
+ * [https://www.freedesktop.org/software/systemd/man/hostname.html]
+ *
+ * For other hostname files the exact format is not clear, but
+ * hostnames cannot begin with "#" and cannot be empty, so ignoring
+ * those lines seems safe.
+ *)
 and check_hostname_from_file filename    let chroot      let name = sprintf
"check_hostname_from_file: %s" filename in
     Chroot.create ~name () in
 
   let hostname = Chroot.f chroot read_small_file filename in
-  match hostname with
-  | None | Some [] | Some [""] -> None
+
+  let keep_line line = line <> "" && not
(String.is_prefix line "#") in
+  let lines = Option.map (List.filter keep_line) hostname in
+  match lines with
+  | None | Some [] -> None
   | Some (hostname :: _) -> Some hostname
 
 (* Parse the hostname from /etc/sysconfig/network.  This must be
-- 
2.37.0.rc2

On 07/20/22 13:45, Richard W.M. Jones wrote:
> How about this instead?
> 
> It actually changes the behaviour slightly.  Currently a /etc/hostname
> which contains the single character '\n' would return
> inspect_get_hostname = "" (which seems wrong).  This is the
default
> /etc/hostname for virt-builder images.
> 
> I think this is because in the current code, read_small_file actually
> returns ?Some [""; ""]?, so it takes the branch where
it matches
> ?| Some (hostname :: _)? which returns empty string.
> 
> Anyway, after this patch it returns "unknown" which is documented
as
> "the hostname could not be determined", which I think is more
correct
> behaviour.  I tested it also on files with blank lines and comments
> and so on, and it seems to do what is necessary, although it could do
> with a proper regression test.
> 
> Rich.
> 
> -- Richard Jones, Virtualization Group, Red Hat
> http://people.redhat.com/~rjones Read my programming and virtualization
> blog: http://rwmj.wordpress.com virt-builder quickly builds VMs from
> scratch http://libguestfs.org/virt-builder.1.html
> 
> 
> 0001-daemon-Parse-etc-hostname-files-containing-comments.patch
> 
> 
> From 3b6897c8ba9c8d438e8599af93188e65cced6ec8 Mon Sep 17 00:00:00 2001
> From: "Richard W.M. Jones" <rjones at redhat.com>
> Date: Wed, 20 Jul 2022 12:33:09 +0100
> Subject: [PATCH] daemon: Parse /etc/hostname files containing comments
> 
> Thanks: Dawid Zamirski
> Link: https://www.freedesktop.org/software/systemd/man/hostname.html
> ---
>  daemon/inspect_fs_unix.ml | 19 ++++++++++++++++---
>  1 file changed, 16 insertions(+), 3 deletions(-)
> 
> diff --git a/daemon/inspect_fs_unix.ml b/daemon/inspect_fs_unix.ml
> index 63cb279d08..8045ef0d43 100644
> --- a/daemon/inspect_fs_unix.ml
> +++ b/daemon/inspect_fs_unix.ml
> @@ -551,15 +551,28 @@ and check_hostname_linux () >       else
>         None
>  
> -(* Parse the hostname where it is stored directly in a file. *)
> +(* Parse the hostname where it is stored directly in a file.
> + *
> + * For /etc/hostname:
> + * "The file should contain a single newline-terminated hostname
> + * string. Comments (lines starting with a "#") are
ignored."
> + * [https://www.freedesktop.org/software/systemd/man/hostname.html]
> + *
> + * For other hostname files the exact format is not clear, but
> + * hostnames cannot begin with "#" and cannot be empty, so
ignoring
> + * those lines seems safe.
> + *)
>  and check_hostname_from_file filename >    let chroot >      let
name = sprintf "check_hostname_from_file: %s" filename in
>      Chroot.create ~name () in
>  
>    let hostname = Chroot.f chroot read_small_file filename in
> -  match hostname with
> -  | None | Some [] | Some [""] -> None
> +
> +  let keep_line line = line <> "" && not
(String.is_prefix line "#") in
> +  let lines = Option.map (List.filter keep_line) hostname in
> +  match lines with
> +  | None | Some [] -> None
>    | Some (hostname :: _) -> Some hostname
>  
>  (* Parse the hostname from /etc/sysconfig/network.  This must be
> -- 2.37.0.rc2
> 
Acked-by: Laszlo Ersek <lersek at redhat.com>

On Wed, 2022-07-20 at 12:45 +0100, Richard W.M. Jones
wrote:
> How about this instead?
> 
> It actually changes the behaviour slightly.? Currently a
> /etc/hostname
> which contains the single character '\n' would return
> inspect_get_hostname = "" (which seems wrong).? This is the
default
> /etc/hostname for virt-builder images.
> 
> I think this is because in the current code, read_small_file actually
> returns ?Some [""; ""]?, so it takes the branch where
it matches
> ?| Some (hostname :: _)? which returns empty string.
> 
> Anyway, after this patch it returns "unknown" which is documented
as
> "the hostname could not be determined", which I think is more
correct
> behaviour.? I tested it also on files with blank lines and comments
> and so on, and it seems to do what is necessary, although it could do
> with a proper regression test.
> 
> Rich.
> 
Works great for me and looks much more elegant that what I came up
with.

Thanks,
Dawid

On Wed, Jul 20, 2022 at 01:36:25PM -0400, dzrudy at gmail.com
wrote:
> On Wed, 2022-07-20 at 12:45 +0100, Richard W.M. Jones wrote:
> > How about this instead?
> > 
> > It actually changes the behaviour slightly.? Currently a
> > /etc/hostname
> > which contains the single character '\n' would return
> > inspect_get_hostname = "" (which seems wrong).? This is the
default
> > /etc/hostname for virt-builder images.
> > 
> > I think this is because in the current code, read_small_file actually
> > returns ?Some [""; ""]?, so it takes the branch
where it matches
> > ?| Some (hostname :: _)? which returns empty string.
> > 
> > Anyway, after this patch it returns "unknown" which is
documented as
> > "the hostname could not be determined", which I think is
more correct
> > behaviour.? I tested it also on files with blank lines and comments
> > and so on, and it seems to do what is necessary, although it could do
> > with a proper regression test.
> > 
> > Rich.
> > 
> 
> Works great for me and looks much more elegant that what I came up
> with.
Thanks - this is upstream commit 4a517601c752955e35b472ea127cb4b7330a6136

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-p2v converts physical machines to virtual machines.  Boot with a
live CD or over the network (PXE) and turn machines into KVM guests.
http://libguestfs.org/virt-v2v