Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

with current main branch, there's regression (network interface names) #1479

Closed
hannesm opened this issue Nov 9, 2023 · 4 comments
Closed
Assignees

Comments

@hannesm
Copy link
Member

hannesm commented Nov 9, 2023

When I have a unikernel that uses the default_network interface, now (with #1455 merged) I get in the generated manifest.json (using mirage configure -t hvt e.g. on the mirage-skeleton/device-usage/network unikernel):

{
  "type": "solo5.manifest",
  "version": 1,
  "devices": [ { "name": "net0", "type": "NET_BASIC" } ]
}

So, the name should be service, not net0. There is in mirage_impl_network a match_impl Key.(value target) -- but I fail to understand how the net0 comes into place (the default is "0").

Previously the generated manifest was:

{
  "type": "solo5.manifest",
  "version": 1,
  "devices": [ { "name": "service", "type": "NET_BASIC" } ]
}
@hannesm
Copy link
Member Author

hannesm commented Nov 9, 2023

I tested with 944a83a (the one before the #1455 merge), and this still works nicely (having service in the manifest.json).

@hannesm
Copy link
Member Author

hannesm commented Nov 9, 2023

The culprit seems to be caabf33, which includes:

--- a/lib/mirage/impl/mirage_impl_network.ml
+++ b/lib/mirage/impl/mirage_impl_network.ml
@@ -1,14 +1,18 @@
 open Functoria
 module Key = Mirage_key
+module Runtime_key = Mirage_runtime_key
 
 type network = NETWORK
 
 let network = Type.v NETWORK
 let all_networks = ref []
 
-let network_conf (intf : string Key.key) =
-  let key = Key.v intf in
-  let keys = [ key ] in
+let add_new_network () =
+  let device = Fmt.str "net%d" (List.length !all_networks) in
+  all_networks := device :: !all_networks
+
+let network_conf (intf : string runtime_key) =
+  let runtime_keys = [ 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" ]
@@ -23,16 +27,15 @@ let network_conf (intf : string Key.key) =
         [ package ~min:"0.8.0" ~max:"0.9.0" "mirage-net-solo5" ]
   in
   let connect _ modname _ =
-    (* @samoht: why not just use the args paramater? *)
-    Fmt.str "%s.connect %a" modname Key.serialize_call key
+    Fmt.str "%s.connect %a" modname Runtime_key.call intf
   in
-  let configure i =
-    all_networks := Key.get (Info.context i) intf :: !all_networks;
+  let configure _ =
+    add_new_network ();
     Action.ok ()
   in
-  impl ~keys ~packages_v ~connect ~configure "Netif" network
+  impl ~runtime_keys ~packages_v ~connect ~configure "Netif" network
 
-let netif ?group dev = network_conf @@ Key.interface ?group dev
+let netif ?group dev = network_conf @@ Runtime_key.interface ?group dev
 
 let default_network =
   match_impl

My gut feeling is that the interface should be a configure-time key!? Hmm, I'm actually not entirely sure:

  • on solo5 (hvt/spt), we specify the name at configuration time (which ends up in the manifest), the value is relevant for solo5-hvt/solo5-spt (i.e. solo5-hvt --net:service=tapX -- dist/network.hvt)
  • virtio supports only a single network interface
  • muen I don't remember the exact semantics
  • on unix/xen, I fail to understand what is happening there!?

@hannesm
Copy link
Member Author

hannesm commented Nov 9, 2023

@samoht please review #1480, I tested it locally with hvt target, and it works nicely. (of course, a test in the testsuite would be welcome, but I fail to understand how to achieve that -- esp. with network devices and block devices and all the different targets)

@hannesm hannesm changed the title with current main branch, there's regression with current main branch, there's regression (network interface names) Nov 9, 2023
@hannesm
Copy link
Member Author

hannesm commented Feb 13, 2024

this has been fixed by #1480 (which was re-submitted as #1492)

@hannesm hannesm closed this as completed Feb 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants