Libguestfs - Oct 2018 - Re: [PATCH API PROPOSAL 0/2] inspection: Add network interfaces to inspection data.

As part of the fix for:

https://bugzilla.redhat.com/show_bug.cgi?id=1626503

I'm proposing to add two new APIs to fetch information about the list
of network interfaces of an existing guest.  These two patches outline
the proposed API but with no implementation or tests.  However they
can be applied and compiled.

Please see the second patch for the proposed API.

I have a mostly working implementation for Red Hat-style ifcfg-*
files, and both Debian /etc/network/interfaces and Windows registry
look do-able using the same architecture.

Rich.

It would be possible to implement this as:

  raise (Unix_error (ENOTSUP, fn, arg))

except that Unix.ENOTSUP is not defined (OCaml 4.07).  Instead of
working around this let's have a special exception, and anyway this
works like the C code.

The only tricky part of this patch is that I wanted to use the
‘Daemon’ module name for daemonic things, such as this exception.  So
I had to rename the existing Daemon module (which contains
initialization code) to ‘Daemon_init’.
---
 daemon/Makefile.am     |  4 +++-
 daemon/daemon-c.c      |  2 ++
 daemon/daemon.ml       | 21 ++++-----------------
 daemon/daemon.mli      |  3 ++-
 daemon/daemon_init.ml  | 38 ++++++++++++++++++++++++++++++++++++++
 daemon/daemon_init.mli | 19 +++++++++++++++++++
 6 files changed, 68 insertions(+), 19 deletions(-)

diff --git a/daemon/Makefile.am b/daemon/Makefile.am
index 5d1c222db..d3adcb53e 100644
--- a/daemon/Makefile.am
+++ b/daemon/Makefile.am
@@ -270,6 +270,7 @@ SOURCES_MLI = \
 	chroot.mli \
 	daemon.mli \
 	daemon_config.mli \
+	daemon_init.mli \
 	devsparts.mli \
 	file.mli \
 	filearch.mli \
@@ -301,6 +302,7 @@ SOURCES_MLI = \
 
 SOURCES_ML = \
 	daemon_config.ml \
+	daemon.ml \
 	utils.ml \
 	structs.ml \
 	optgroups.ml \
@@ -333,7 +335,7 @@ SOURCES_ML = \
 	inspect_fs.ml \
 	inspect.ml \
 	callbacks.ml \
-	daemon.ml
+	daemon_init.ml
 
 BOBJECTS = $(SOURCES_ML:.ml=.cmo)
 XOBJECTS = $(BOBJECTS:.cmo=.cmx)
diff --git a/daemon/daemon-c.c b/daemon/daemon-c.c
index 533bf7ce7..b347ade81 100644
--- a/daemon/daemon-c.c
+++ b/daemon/daemon-c.c
@@ -59,6 +59,8 @@ guestfs_int_daemon_exn_to_reply_with_error (const char *func,
value exn)
                              String_val (Field (exn, 2)),
                              String_val (Field (exn, 3)));
   }
+  else if (STREQ (exn_name, "Not_supported"))
+    reply_with_error_errno (ENOTSUP, "%s", String_val (Field (exn,
1)));
   else if (STREQ (exn_name, "Failure"))
     reply_with_error ("%s", String_val (Field (exn, 1)));
   else if (STREQ (exn_name, "Sys_error"))
diff --git a/daemon/daemon.ml b/daemon/daemon.ml
index f24d64a13..132bc6a21 100644
--- a/daemon/daemon.ml
+++ b/daemon/daemon.ml
@@ -18,21 +18,8 @@
 
 open Printf
 
-(* When guestfsd starts up, early on (after parsing the command line
- * but not much else), it calls 'caml_startup' which runs all
- * initialization code in the OCaml modules, including this one.
- *
- * Therefore this is where we can place OCaml initialization code
- * for the daemon.
- *)
+exception Not_supported of string
+
 let () -  (* Connect the guestfsd [-v] (verbose) flag into 'verbose ()'
-   * used in OCaml code to print debugging messages.
-   *)
-  if Utils.get_verbose_flag () then (
-    Std_utils.set_verbose ();
-    eprintf "OCaml daemon loaded\n%!"
-  );
-
-  (* Register the callbacks which are used to call OCaml code from C. *)
-  Callbacks.init_callbacks ()
+  (* Register exceptions. *)
+  Callback.register_exception "Not_supported" (Not_supported
"")
diff --git a/daemon/daemon.mli b/daemon/daemon.mli
index c893b4a1e..856364a78 100644
--- a/daemon/daemon.mli
+++ b/daemon/daemon.mli
@@ -16,4 +16,5 @@
  * 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA.
  *)
 
-(* Nothing is exported. *)
+exception Not_supported of string
+(** Like the [NOT_SUPPORTED] macro in [daemon.h]. *)
diff --git a/daemon/daemon_init.ml b/daemon/daemon_init.ml
new file mode 100644
index 000000000..f24d64a13
--- /dev/null
+++ b/daemon/daemon_init.ml
@@ -0,0 +1,38 @@
+(* guestfs-inspection
+ * Copyright (C) 2009-2018 Red Hat Inc.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License along
+ * with this program; if not, write to the Free Software Foundation, Inc.,
+ * 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA.
+ *)
+
+open Printf
+
+(* When guestfsd starts up, early on (after parsing the command line
+ * but not much else), it calls 'caml_startup' which runs all
+ * initialization code in the OCaml modules, including this one.
+ *
+ * Therefore this is where we can place OCaml initialization code
+ * for the daemon.
+ *)
+let () +  (* Connect the guestfsd [-v] (verbose) flag into 'verbose ()'
+   * used in OCaml code to print debugging messages.
+   *)
+  if Utils.get_verbose_flag () then (
+    Std_utils.set_verbose ();
+    eprintf "OCaml daemon loaded\n%!"
+  );
+
+  (* Register the callbacks which are used to call OCaml code from C. *)
+  Callbacks.init_callbacks ()
diff --git a/daemon/daemon_init.mli b/daemon/daemon_init.mli
new file mode 100644
index 000000000..c893b4a1e
--- /dev/null
+++ b/daemon/daemon_init.mli
@@ -0,0 +1,19 @@
+(* guestfsd
+ * Copyright (C) 2009-2018 Red Hat Inc.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License along
+ * with this program; if not, write to the Free Software Foundation, Inc.,
+ * 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA.
+ *)
+
+(* Nothing is exported. *)
-- 
2.19.0.rc0

This adds network interfaces to inspection data.

The data is returned as a flat list of structs containing a general
overview of the interfaces, and a separate call returning a fairly
arbitrary list of key/value parameters for each interface.  The list
of parameters is roughly modelled on the ifcfg-* files, although with
some mapping of the data to make it easier to consume.

Note this doesn't attempt a detailed model of all possible guest
network configurations, which in particular cases could be extremely
complex.

This commits just throws ENOTSUP error for all guests.  Following
commits add this information for Red Hat-derived, Debian-derived and
Windows guests.
---
 daemon/inspect.ml                        | 18 +++++
 daemon/inspect_types.ml                  | 19 +++++-
 daemon/inspect_types.mli                 |  3 +
 generator/actions_inspection.ml          | 86 ++++++++++++++++++++++++
 generator/proc_nr.ml                     |  2 +
 generator/structs.ml                     | 10 +++
 gobject/Makefile.inc                     |  2 +
 java/Makefile.inc                        |  1 +
 java/com/redhat/et/libguestfs/.gitignore |  1 +
 lib/MAX_PROC_NR                          |  2 +-
 10 files changed, 142 insertions(+), 2 deletions(-)

diff --git a/daemon/inspect.ml b/daemon/inspect.ml
index ce62c17f2..0cb1aa5d8 100644
--- a/daemon/inspect.ml
+++ b/daemon/inspect.ml
@@ -376,6 +376,24 @@ and inspect_get_drive_mappings root    let root =
search_for_root root in
   root.inspection_data.drive_mappings
 
+and inspect_get_interfaces root +  let root = search_for_root root in
+  match root.inspection_data.interfaces with
+  | None ->
+     raise (Daemon.Not_supported "guest type not supported")
+  | Some interfaces -> List.map fst interfaces
+
+and inspect_get_interface_parameters root i +  let root = search_for_root root
in
+  match root.inspection_data.interfaces with
+  | None ->
+     raise (Daemon.Not_supported "guest type not supported")
+  | Some interfaces ->
+     let interfaces = List.map snd interfaces in
+     try List.nth interfaces i
+     with Failure _ | Invalid_argument _ ->
+       failwith "index out of range"
+
 and search_for_root root    let fses = !Inspect_types.inspect_fses in
   if fses = [] then
diff --git a/daemon/inspect_types.ml b/daemon/inspect_types.ml
index 70b34c51c..37592275b 100644
--- a/daemon/inspect_types.ml
+++ b/daemon/inspect_types.ml
@@ -54,6 +54,7 @@ and inspection_data = {
   mutable windows_system_hive : string option;
   mutable windows_current_control_set : string option;
   mutable drive_mappings : drive_mapping list;
+  mutable interfaces : interfaces option;
 }
 and os_type    | OS_TYPE_DOS
@@ -125,6 +126,8 @@ and package_management  and version = int * int
 and fstab_entry = Mountable.t * string (* mountable, mountpoint *)
 and drive_mapping = string * string (* drive name, device *)
+and interfaces = interface list
+and interface = Structs.interface * (string * string) list
 
 let rec string_of_fs { fs_location = location; role }    sprintf "fs: %s
role: %s\n"
@@ -183,6 +186,17 @@ and string_of_inspection_data data        List.map (fun (a,
b) -> sprintf "(%s, %s)" a b) data.drive_mappings in
     bpf "    drive_mappings: [%s]\n" (String.concat ", " v)
   );
+  Option.may (
+    fun interfaces ->
+      bpf "    interfaces:\n";
+      List.iteri (
+        fun i ({ Structs.if_type; if_hwaddr; if_name }, params) ->
+          bpf "      [%d]: type: %s mac: %s name: %s"
+              i if_type if_hwaddr if_name;
+          List.iter (fun (k, v) -> bpf " %s: %s" k v) params;
+          bpf "\n"
+      ) interfaces
+  ) data.interfaces;
   Buffer.contents b
 
 and string_of_os_type = function
@@ -272,6 +286,7 @@ let null_inspection_data = {
   windows_system_hive = None;
   windows_current_control_set = None;
   drive_mappings = [];
+  interfaces = None;
 }
 let null_inspection_data () = { null_inspection_data with os_type = None }
 
@@ -299,7 +314,9 @@ let merge_inspection_data child parent      merge
child.windows_current_control_set parent.windows_current_control_set;
 
   (* This is what the old C code did, but I doubt that it's correct. *)
-  parent.drive_mappings <-  child.drive_mappings @ parent.drive_mappings
+  parent.drive_mappings <-  child.drive_mappings @ parent.drive_mappings;
+
+  parent.interfaces <-      merge child.interfaces parent.interfaces
 
 let merge child_fs parent_fs    let inspection_data_of_fs = function
diff --git a/daemon/inspect_types.mli b/daemon/inspect_types.mli
index 7493aa3a6..e82b2a554 100644
--- a/daemon/inspect_types.mli
+++ b/daemon/inspect_types.mli
@@ -57,6 +57,7 @@ and inspection_data = {
   mutable windows_system_hive : string option;
   mutable windows_current_control_set : string option;
   mutable drive_mappings : drive_mapping list;
+  mutable interfaces : interfaces option;
 }
 (** During inspection, this data is collected incrementally for each
     filesystem.  At the end of inspection, inspection data is merged
@@ -132,6 +133,8 @@ and package_management  and version = int * int
 and fstab_entry = Mountable.t * string (* mountable, mountpoint *)
 and drive_mapping = string * string (* drive name, device *)
+and interfaces = interface list
+and interface = Structs.interface * (string * string) list
 
 val merge_inspection_data : inspection_data -> inspection_data -> unit
 (** [merge_inspection_data child parent] merges two sets of inspection
diff --git a/generator/actions_inspection.ml b/generator/actions_inspection.ml
index d913f53e3..a4ca437ff 100644
--- a/generator/actions_inspection.ml
+++ b/generator/actions_inspection.ml
@@ -603,6 +603,92 @@ Please read L<guestfs(3)/INSPECTION> for more
details.
 See also C<guestfs_inspect_get_mountpoints>,
 C<guestfs_inspect_get_filesystems>." };
 
+  { defaults with
+    name = "inspect_get_interfaces"; added = (1, 39, 11);
+    style = RStructList ("interfaces", "interface"),
[String (Mountable, "root")], [];
+    impl = OCaml "Inspect.inspect_get_interfaces";
+    shortdesc = "get the list of network interfaces";
+    longdesc = "\
+Return the list of network interfaces seen by this guest.  This returns
+a list of structs, one struct per network interface.
+
+The returned C<interface> struct contains only a few fields common
+to all interfaces.  For detailed parameters for each interface
+you also need to call C<guestfs_inspect_get_interface_parameters>
+with the index of the interface you are interested in.
+
+The returned C<interface> struct contains these fields:
+
+=over 4
+
+=item C<if_type>
+
+Interface type, including C<ethernet> (wired ethernet), C<wireless>
+(WiFi).  Other interface types are possible.
+
+=item C<if_hwaddr>
+
+The hardware address (MAC address) of the interface if one is known,
+otherwise the empty string.
+
+=item C<if_name>
+
+The name of the interface as it is known to the guest (eg. C<eth0>).
+Note that network interface names might not be unique.
+
+=back
+
+Depending on the guest type this call may return more network interfaces
+than the guest has physical devices.  In particular it may return all
+network interfaces ever seen by the guest across multiple boots.  To
+work out which interfaces correspond to currently available devices
+you will have to use MAC address information derived from another
+source (eg. hypervisor metadata) and match that to C<if_hwaddr> from
+the struct.
+
+If inspection of network interfaces is not implemented for this
+particular guest type then this will fail with an error and set
+C<errno> to C<ENOTSUP>, so callers should be careful to check for
that
+case." };
+
+  { defaults with
+    name = "inspect_get_interface_parameters"; added = (1, 39, 11);
+    style = RHashtable (RPlainString, RPlainString, "parameters"),
[String (Mountable, "root"); Int "index"], [];
+    impl = OCaml "Inspect.inspect_get_interface_parameters";
+    shortdesc = "get detailed configuration parameters for a network
interface";
+    longdesc = "\
+Return the detailed configuration parameters of the C<index> network
+interface.  Indexes refer to the list returned by
+C<guestfs_inspect_get_interfaces> and count from zero.
+
+The returned parameters can contain a variety of keys.  Some notable
+ones are described below:
+
+=over 4
+
+=item C<bootproto>
+
+C<dhcp> if the interface was last configured to find an IP address
+through DHCP, else C<none> if it was configured to use a static
+IP address.  Other values may be possible.
+
+=item C<essid>
+
+For wireless interfaces, the ESSID.
+
+=item C<ipaddr>, C<network>, C<netmask>, C<gateway>,
C<broadcast>
+
+IPv4 address if configured statically.  For some (but not all)
+guest types if the interface uses DHCP then this may record the
+previous IPv4 address which was acquired through DHCP.
+
+=item C<onboot>
+
+Key is present and value is C<1> if the network interface is configured
+to come up at boot.
+
+=back" };
+
 ]
 
 let non_daemon_functions = [
diff --git a/generator/proc_nr.ml b/generator/proc_nr.ml
index 2adf8a32f..4781f0a24 100644
--- a/generator/proc_nr.ml
+++ b/generator/proc_nr.ml
@@ -514,6 +514,8 @@ let proc_nr = [
 504, "part_get_gpt_attributes";
 505, "f2fs_expand";
 506, "lvm_scan";
+507, "inspect_get_interfaces";
+508, "inspect_get_interface_parameters";
 ]
 
 (* End of list.  If adding a new entry, add it at the end of the list
diff --git a/generator/structs.ml b/generator/structs.ml
index 14ee2813f..09e90bd0b 100644
--- a/generator/structs.ml
+++ b/generator/structs.ml
@@ -478,6 +478,16 @@ let structs = [
     ];
     s_camel_name = "YaraDetection" };
 
+  (* Network interfaces. *)
+  { defaults with
+    s_name = "interface";
+    s_cols = [
+    "if_type", FString;
+    "if_hwaddr", FString;
+    "if_name", FString;
+    ];
+    s_camel_name = "Interface" };
+
 ] (* end of structs *)
 
 let lookup_struct name diff --git a/gobject/Makefile.inc b/gobject/Makefile.inc
index 5aa2dcafe..42839a85f 100644
--- a/gobject/Makefile.inc
+++ b/gobject/Makefile.inc
@@ -35,6 +35,7 @@ guestfs_gobject_headers= \
   include/guestfs-gobject/struct-hivex_value.h \
   include/guestfs-gobject/struct-inotify_event.h \
   include/guestfs-gobject/struct-int_bool.h \
+  include/guestfs-gobject/struct-interface.h \
   include/guestfs-gobject/struct-isoinfo.h \
   include/guestfs-gobject/struct-lvm_lv.h \
   include/guestfs-gobject/struct-lvm_pv.h \
@@ -128,6 +129,7 @@ guestfs_gobject_sources= \
   src/struct-hivex_value.c \
   src/struct-inotify_event.c \
   src/struct-int_bool.c \
+  src/struct-interface.c \
   src/struct-isoinfo.c \
   src/struct-lvm_lv.c \
   src/struct-lvm_pv.c \
diff --git a/java/Makefile.inc b/java/Makefile.inc
index 93ec5e129..54b9da971 100644
--- a/java/Makefile.inc
+++ b/java/Makefile.inc
@@ -33,6 +33,7 @@ java_built_sources = \
 	com/redhat/et/libguestfs/INotifyEvent.java \
 	com/redhat/et/libguestfs/ISOInfo.java \
 	com/redhat/et/libguestfs/IntBool.java \
+	com/redhat/et/libguestfs/Interface.java \
 	com/redhat/et/libguestfs/LV.java \
 	com/redhat/et/libguestfs/MDStat.java \
 	com/redhat/et/libguestfs/PV.java \
diff --git a/java/com/redhat/et/libguestfs/.gitignore
b/java/com/redhat/et/libguestfs/.gitignore
index bc03cb965..b5843c8f1 100644
--- a/java/com/redhat/et/libguestfs/.gitignore
+++ b/java/com/redhat/et/libguestfs/.gitignore
@@ -10,6 +10,7 @@ HivexValue.java
 INotifyEvent.java
 ISOInfo.java
 IntBool.java
+Interface.java
 LV.java
 MDStat.java
 PV.java
diff --git a/lib/MAX_PROC_NR b/lib/MAX_PROC_NR
index 80e3e6eab..1eccde110 100644
--- a/lib/MAX_PROC_NR
+++ b/lib/MAX_PROC_NR
@@ -1 +1 @@
-506
+508
-- 
2.19.0.rc0

On Monday, 1 October 2018 13:28:26 CEST Richard W.M. Jones
wrote:
> As part of the fix for:
> 
> https://bugzilla.redhat.com/show_bug.cgi?id=1626503
> 
> I'm proposing to add two new APIs to fetch information about the list
> of network interfaces of an existing guest.  These two patches outline
> the proposed API but with no implementation or tests.  However they
> can be applied and compiled.
> 
> Please see the second patch for the proposed API.
> 
> I have a mostly working implementation for Red Hat-style ifcfg-*
> files, and both Debian /etc/network/interfaces and Windows registry
> look do-able using the same architecture.
It is hard to see whether this API is enough to cover the use case,
without an actual implementation.

I remember RFCs for virt-builder/virt-customize to change the network
configuration of a guest, and even just determining the actual
configuration that a guest will have is very hard: you can have
multiple network-related daemon installed in the guest, different
available configurations (even stale ones, if they were not cleaned
when their system was removed), and so on.
Hence, I am notsure whether trying to get any data is better than just
not provide any at all...

-- 
Pino Toscano

On Mon, Oct 01, 2018 at 07:00:17PM +0200, Pino Toscano
wrote:
> On Monday, 1 October 2018 13:28:26 CEST Richard W.M. Jones wrote:
> > As part of the fix for:
> > 
> > https://bugzilla.redhat.com/show_bug.cgi?id=1626503
> > 
> > I'm proposing to add two new APIs to fetch information about the
list
> > of network interfaces of an existing guest.  These two patches outline
> > the proposed API but with no implementation or tests.  However they
> > can be applied and compiled.
> > 
> > Please see the second patch for the proposed API.
> > 
> > I have a mostly working implementation for Red Hat-style ifcfg-*
> > files, and both Debian /etc/network/interfaces and Windows registry
> > look do-able using the same architecture.
> 
> It is hard to see whether this API is enough to cover the use case,
> without an actual implementation.
> 
> I remember RFCs for virt-builder/virt-customize to change the network
> configuration of a guest, and even just determining the actual
> configuration that a guest will have is very hard: you can have
> multiple network-related daemon installed in the guest, different
> available configurations (even stale ones, if they were not cleaned
> when their system was removed), and so on.
> Hence, I am notsure whether trying to get any data is better than just
> not provide any at all...
I posted v2 of this patch last night which includes a single working
implementation, but I also meant to reply to this message with some
more detail.

I'm not expecting we'll be able to model every detail of guest network
configuration.  However a simple list of interfaces is useful for
several purposes:

* The virt-v2v bug (see above).

* Answering the "what IP does the guest have" question which comes up
  again and again.

* A way to tie external devices presented by the hypervisor to
  internal device drivers provided by the guest.

For Linux it's true that multiple network services can be installed,
although only one is likely to be used and it's not too much work to
find out which one is used (although the current patch set does punt
on this question and makes some assumptions).  For Windows the answer
is much clearer and simpler: there's just a list of network devices in
the registry and that's what we present through the inspection data.

Another thing which may not be clear from the patch posted: The list
of parameters should be canonicalized, so that for example the Windows
registry key "IPAddress" is translated to "ipaddr".  In all
cases I'll
try to use the name of the thing used by the ifcfg-* files.

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