Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
88 changes: 42 additions & 46 deletions src/executor/device.rs
Original file line number Diff line number Diff line change
@@ -1,5 +1,4 @@
use alloc::boxed::Box;
#[cfg(not(feature = "dhcpv4"))]
use core::str::FromStr;

use smoltcp::iface::{Config, Interface, SocketSet};
Expand All @@ -8,11 +7,9 @@ use smoltcp::phy::Tracer;
use smoltcp::phy::{Device, Medium};
#[cfg(feature = "dhcpv4")]
use smoltcp::socket::dhcpv4;
#[cfg(all(feature = "dns", not(feature = "dhcpv4")))]
#[cfg(feature = "dns")]
use smoltcp::socket::dns;
use smoltcp::wire::{EthernetAddress, HardwareAddress};
#[cfg(not(feature = "dhcpv4"))]
use smoltcp::wire::{IpCidr, Ipv4Address, Ipv4Cidr};
use smoltcp::wire::{EthernetAddress, HardwareAddress, IpCidr, Ipv4Address, Ipv4Cidr};

use super::network::{NetworkInterface, NetworkState};
use crate::arch;
Expand Down Expand Up @@ -73,52 +70,51 @@ impl<'a> NetworkInterface<'a> {
config.hardware_addr = hardware_addr;
}

#[cfg_attr(feature = "dhcpv4", expect(unused_mut))]
let mut iface = Interface::new(config, &mut device, crate::executor::network::now());
#[cfg_attr(all(not(feature = "dhcpv4"), not(feature = "dns")), expect(unused_mut))]
let mut sockets = SocketSet::new(vec![]);

cfg_select! {
feature = "dhcpv4" => {
if let Some(hermit_ip) = hermit_var!("HERMIT_IP") {
warn!("HERMIT_IP was set to {hermit_ip}, but Hermit was built with DHCPv4.");
warn!("Ignoring HERMIT_IP.");
}

let dhcp = dhcpv4::Socket::new();
let dhcp_handle = sockets.add(dhcp);
#[cfg(feature = "dns")]
let dns_handle = None;
}
_ => {
let myip = Ipv4Address::from_str(hermit_var_or!("HERMIT_IP", "10.0.5.3")).unwrap();
let mygw =
Ipv4Address::from_str(hermit_var_or!("HERMIT_GATEWAY", "10.0.5.1")).unwrap();
let mymask =
Ipv4Address::from_str(hermit_var_or!("HERMIT_MASK", "255.255.255.0")).unwrap();

let ip_addr = IpCidr::from(Ipv4Cidr::from_netmask(myip, mymask).unwrap());
info!("IP address: {ip_addr}");
info!("Gateway: {mygw}");

iface.update_ip_addrs(|ip_addrs| {
ip_addrs.push(ip_addr).unwrap();
});
iface.routes_mut().add_default_ipv4_route(mygw).unwrap();

#[cfg(feature = "dns")]
let dns_handle = {
// Quad9 DNS server
let mydns1 =
Ipv4Address::from_str(hermit_var_or!("HERMIT_DNS1", "9.9.9.9")).unwrap();
// Cloudflare DNS server
let mydns2 =
Ipv4Address::from_str(hermit_var_or!("HERMIT_DNS2", "1.1.1.1")).unwrap();
let servers = &[mydns1.into(), mydns2.into()];
let dns_socket = dns::Socket::new(servers, vec![]);
Some(sockets.add(dns_socket))
};
#[cfg(feature = "dns")]
let mut dns_handle = None;

#[cfg(feature = "dhcpv4")]
let dhcp_handle = {
if let Some(hermit_ip) = hermit_var!("HERMIT_IP") {
warn!("HERMIT_IP was set to {hermit_ip}, but Hermit was built with DHCPv4.");
warn!(
"HERMIT_IP will be overwritten if a DHCP configuration is acquired. If the provided configuration was not meant to be a fallback, disable the DHCP feature."
);
}
sockets.add(dhcpv4::Socket::new())
};

if !cfg!(feature = "dhcpv4") || hermit_var!("HERMIT_IP").is_some() {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if !cfg!(feature = "dhcpv4") || hermit_var!("HERMIT_IP").is_some() {
if cfg!(not(feature = "dhcpv4")) || hermit_var!("HERMIT_IP").is_some() {

Is there a reason we don't want to fall back to 10.0.5.3 if no DHCP configuration is acquired? I feel like that would simplify the logic a bit, and the way I see it, HERMIT_IP is supposed to overwrite the 10.0.5.3 default, having it also imply enabling the fallback is a bit unintuitive. I wouldn't mind always falling back to the static configuration.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My impression is that it is not good practice to claim an IP address without explicit input from the user. When DHCP is not enabled, it is reasonable to infer that the user wants a static IP and if one is not provided use the default, but if DHCP is enabled we shouldn't claim an address without the user asking for it.

Always falling back to the default would also mean that the user has no way of preventing the kernel from claiming an address. I guess we could make it so that when HERMIT_IP is set to something like 0.0.0.0 we don't set an address, but I think the behaviour in the PR is safer.

For the suggested change, I personally prefer to stay in the normal Rust land as opposed to the configuration specification language as much as possible, but I also don't really mind the alternative.

let myip = Ipv4Address::from_str(hermit_var_or!("HERMIT_IP", "10.0.5.3")).unwrap();
let mygw = Ipv4Address::from_str(hermit_var_or!("HERMIT_GATEWAY", "10.0.5.1")).unwrap();
let mymask =
Ipv4Address::from_str(hermit_var_or!("HERMIT_MASK", "255.255.255.0")).unwrap();

let ip_addr = IpCidr::from(Ipv4Cidr::from_netmask(myip, mymask).unwrap());
info!("IP address: {ip_addr}");
info!("Gateway: {mygw}");

iface.update_ip_addrs(|ip_addrs| {
ip_addrs.push(ip_addr).unwrap();
});
iface.routes_mut().add_default_ipv4_route(mygw).unwrap();

#[cfg(feature = "dns")]
{
// Quad9 DNS server
let mydns1 =
Ipv4Address::from_str(hermit_var_or!("HERMIT_DNS1", "9.9.9.9")).unwrap();
// Cloudflare DNS server
let mydns2 =
Ipv4Address::from_str(hermit_var_or!("HERMIT_DNS2", "1.1.1.1")).unwrap();
let servers = &[mydns1.into(), mydns2.into()];
let dns_socket = dns::Socket::new(servers, vec![]);
dns_handle = Some(sockets.add(dns_socket));
};
}

NetworkState::Initialized(Box::new(Self {
Expand Down
11 changes: 10 additions & 1 deletion src/executor/network.rs
Original file line number Diff line number Diff line change
Expand Up @@ -113,6 +113,7 @@ pub(crate) fn now() -> Instant {

#[cfg(feature = "dhcpv4")]
async fn dhcpv4_run() {
let mut was_ever_configured = false;
future::poll_fn(|cx| {
let Some(mut guard) = NIC.try_lock() else {
// FIXME: only wake when progress can be made
Expand All @@ -130,13 +131,14 @@ async fn dhcpv4_run() {
None => {}
Some(dhcpv4::Event::Configured(config)) => {
info!("DHCP config acquired!");
info!("IP address: {}", config.address);
nic.iface.update_ip_addrs(|addrs| {
if let Some(dest) = addrs.iter_mut().next() {
warn!("Overwriting the default network interface configuration.");
*dest = IpCidr::Ipv4(config.address);
} else if addrs.push(IpCidr::Ipv4(config.address)).is_err() {
info!("Unable to update IP address");
}
info!("IP address: {}", config.address);
});
if let Some(router) = config.router {
info!("Gateway: {router}");
Expand All @@ -162,8 +164,15 @@ async fn dhcpv4_run() {
let dns_socket = dns::Socket::new(dns_servers.as_slice(), vec![]);
nic.dns_handle = Some(nic.sockets.add(dns_socket));
}

was_ever_configured = true;
}
Some(dhcpv4::Event::Deconfigured) => {
if !was_ever_configured {
// If there is a default configuration, we do not want to reset it. If there is not, there is not a need to reset it.
return Poll::Pending;
}

info!("DHCP lost config!");
let cidr = Ipv4Cidr::new(Ipv4Address::UNSPECIFIED, 0);
nic.iface.update_ip_addrs(|addrs| {
Expand Down
13 changes: 9 additions & 4 deletions xtask/src/ci/qemu.rs
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,8 @@ use xshell::cmd;

use crate::arch::Arch;

const DEFAULT_GUEST_IP: IpAddr = IpAddr::V4(Ipv4Addr::new(10, 0, 5, 3));

/// Run image on QEMU.
#[derive(Args)]
pub struct Qemu {
Expand Down Expand Up @@ -497,12 +499,15 @@ impl Qemu {
}

fn kernel_args(&self) -> Vec<String> {
let mut args = vec![];
if self.microvm {
let frequency = get_frequency();
vec!["-freq".to_owned(), frequency.to_string()]
} else {
vec![]
args.extend(["-freq".to_owned(), frequency.to_string()]);
}
if self.tap {
args.extend(["-ip".to_owned(), DEFAULT_GUEST_IP.to_string()]);
}
args
}

fn app_args(&self, image_name: &str) -> Vec<String> {
Expand All @@ -529,7 +534,7 @@ impl Qemu {
if let Ok(ip) = env::var("HERMIT_IP") {
ip.parse().unwrap()
} else {
Ipv4Addr::new(10, 0, 5, 3).into()
DEFAULT_GUEST_IP
}
} else {
Ipv4Addr::LOCALHOST.into()
Expand Down
Loading