provide actual network interface name (regression in #1455)

[hannesm]

fixes #1479

[samoht]

LGTM but needs a dune fmt --auto

[hannesm]

@samoht thanks, I autoformatted

[hannesm]

There's still the underlying issue (can be solved separately): the block device and network devices are runtime-keys for the unix backend (and macos as well); but for solo5, since they're part of the manifest they are configuration time, and the --interface / --interface-management should disappear from the solo5 targets).

Now, a brief look into how the runtime keys are collected, I don't see an easy path to have a key only on some targets (but I may just need to do some more reading).

[reynir]

I did an attempt at making --interface configure-time for solo5 targets:
https://github.com/reynir/mirage/tree/fix-1479

It seems to work fine except mirage configure -t hvt --help doesn't list the --interface option. The output in the mirage describe test is also different and maybe not as expected. I don't quite understand mirage describe.

File "test/mirage/describe/run.t", line 1, characters 0-0:
diff --git a/_build/.sandbox/2b3343007fc450d2b17353409a25d7d8/default/test/mirage/describe/run.t b/_build/.sandbox/2b3343007fc450d2b17353409a25d7d8/default/test/mirage/describe/run.t.corrected
index 097259f885..a82612e900 100644
--- a/_build/.sandbox/2b3343007fc450d2b17353409a25d7d8/default/test/mirage/describe/run.t
+++ b/_build/.sandbox/2b3343007fc450d2b17353409a25d7d8/default/test/mirage/describe/run.t.corrected
@@ -3,16 +3,34 @@
 Describe before configure (using defaults)
   $ ./config.exe describe -t spt
   Name       describe
-  Keys       dhcp=false (default),
-             net= (default),
-             target=spt
+  Keys      
+    dhcp=false (default),
+    interface=service (default),
+    interface=service (default),
+    interface=service (default),
+    interface=service (default),
+    interface=service (default),
+    interface=tap0 (default),
+    interface=tap0 (default),
+    interface=0 (default),
+    net= (default),
+    target=spt
 
 Describe before configure (no eval)
   $ ./config.exe describe --no-eval --dot -o-
   Name       describe
-  Keys       dhcp=false (default),
-             net= (default),
-             target=unix (default)
+  Keys      
+    dhcp=false (default),
+    interface=service (default),
+    interface=service (default),
+    interface=service (default),
+    interface=service (default),
+    interface=service (default),
+    interface=tap0 (default),
+    interface=tap0 (default),
+    interface=0 (default),
+    net= (default),
+    target=unix (default)
   Output     -digraph G {
                 ordering=out;
                 1 [label="tcpv4v6_socket__1\nTcpv4v6_socket\n", shape="box"];
@@ -24,42 +42,58 @@ Describe before configure (no eval)
[...SNIP dot output SNIP]

The strategy is to have a runtime_network_conf (which is just this PR's network_conf renamed) and a network_conf for config time which is roughly the network_conf as before the configuration rework. Then in netif we do if_impl Key.is_solo5 (network_conf ...) (runtime_network_conf ...).

[hannesm]

What I had in mind was to remove the --interface from solo5 completely... This means applying the diff:

diff --git a/lib/mirage/impl/mirage_impl_network.ml b/lib/mirage/impl/mirage_impl_network.ml
index 380f0db3..338ffea0 100644
--- a/lib/mirage/impl/mirage_impl_network.ml
+++ b/lib/mirage/impl/mirage_impl_network.ml
@@ -8,8 +8,8 @@ let network = Type.v NETWORK
 let all_networks = ref []
 let add_new_network name = all_networks := name :: !all_networks
 
-let network_conf name (intf : string runtime_key) =
-  let runtime_keys = [ Runtime_key.v intf ] in
+let network_conf ?(intf : string runtime_key option) name =
+  let runtime_keys = Option.to_list (Option.map Runtime_key.v intf) in
   let packages_v =
     Key.match_ Key.(value target) @@ function
     | `Unix -> [ package ~min:"3.0.0" ~max:"4.0.0" "mirage-net-unix" ]
@@ -24,7 +24,11 @@ let network_conf name (intf : string runtime_key) =
         [ package ~min:"0.8.0" ~max:"0.9.0" "mirage-net-solo5" ]
   in
   let connect _ modname _ =
-    Fmt.str "%s.connect %a" modname Runtime_key.call intf
+    let name =
+      Option.value ~default:(String.escaped name)
+        (Option.map (Fmt.to_to_string Runtime_key.call) intf)
+    in
+    Fmt.str "%s.connect %s" modname name
   in
   let configure _ =
     add_new_network name;
@@ -32,7 +36,9 @@ let network_conf name (intf : string runtime_key) =
   in
   impl ~runtime_keys ~packages_v ~connect ~configure "Netif" network
 
-let netif ?group dev = network_conf dev @@ Runtime_key.interface ?group dev
+let netif ?group dev =
+  if_impl Key.is_solo5 (network_conf dev)
+    (network_conf ~intf:(Runtime_key.interface ?group dev) dev)
 
 let default_network =
   match_impl

But there's some change in the runtest / dot output that I do not quite understand.

[hannesm]

I rebased on the main branch, and force-pushed this PR. It now includes the change to only have --interface runtime keys on non-solo5 targets.

I tested with unix and hvt, and looked into the muen code. These three targets should be fine. I still fail to have a xen host available, and thus am unsure what the xen/qubes story is (and whether this PR is good for these environments?).

[hannesm]

The change in the "test/mirage/describe/run.t", I have honestly no clue what this test is doing, and whether the diff provided in the last commit is good or not.

[hannesm]

an approval for this PR would be nice. For me, it fixes actual issues (and avoids generating new random names). :)

[reynir]

To me this looks good. I think for xen it is fine although it would be good if someone could test or someone more familiar with the xen target confirm it looks good. For the qubes target it seems to me it is layer 3 (IP) so this change is only relevant if you use the xen target for qubes - in which case it shouldn't matter if it's qubes or another xen target.

[hannesm]

I'm unsure about this PR and the main branch of mirage. I'm pretty sure others who understand the code better should just fix the issue described in 1479. Closing.

[samoht]

The changes looks good to me --as you've deleted the branch, I've pushed a copy here: #1492

The changes to run.t are expected, as you are changing the DSL term (with a new if_impl -- this adds a new comparison node in the graph.