[PATCH] Add simple parsing logic for hostname file.
There are some linux systems in the wild where the /etc/hostname file does not contain the configured hostname as sole line in that file. This file actually allows for comments [1] starting with "#" and the original logic would extract the comment line instead of the hostname on such systems. This patch fixes this by filtering out the comment lines and any empty lines. [1] https://www.freedesktop.org/software/systemd/man/hostname.html --- Please keep in mind that this is my first attempt at OCaml after taking a quick crash-course to get familiar with the language to produce this patch. Therefore, the patch may be very suboptimal and break guidelines, so please be forgiving :-) daemon/inspect_fs_unix.ml | 22 +++++++++++++++++----- 1 file changed, 17 insertions(+), 5 deletions(-) diff --git a/daemon/inspect_fs_unix.ml b/daemon/inspect_fs_unix.ml index 63cb279d0..533ef71b2 100644 --- a/daemon/inspect_fs_unix.ml +++ b/daemon/inspect_fs_unix.ml @@ -556,11 +556,23 @@ 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 - | Some (hostname :: _) -> Some hostname + try + let lines = Chroot.f chroot read_small_file filename in + let lines = match lines with + | None -> raise Not_found + | Some lines -> List.map String.trim lines in + let rec loop = function + | [] -> raise Not_found + | line :: _ when String.length line > 0 && + not (String.is_prefix line "#") -> + line + | _ :: lines -> + loop lines + in + let hostname = loop lines in + Some hostname + with + Not_found -> None (* Parse the hostname from /etc/sysconfig/network. This must be * called from the 'with_augeas' wrapper. Note that F18+ and -- 2.36.1
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
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@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@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@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
participants (4)
-
Dawid Zamirski -
dzrudy＠gmail.com
-
Laszlo Ersek -
Richard W.M. Jones