fix(virtio-net): waiting infinetely for bridge to be created (#47)

* fix: propagates async from bridge to VMM remove futures block_on in bridge module, because they're not polled (so not executed) Signed-off-by: Mateo Fernandez <[email protected]> * nitpick: add error log because not in the error returned to gRPC Signed-off-by: Mateo Fernandez <[email protected]> * doc: add "iptables" to dependencies and "just" as optional dependency Signed-off-by: Mateo Fernandez <[email protected]> * fix: create addr for bridge with check on Result<Option<>> Signed-off-by: Mateo Fernandez <[email protected]> * fix: remove unnecessary async in VMM::new Signed-off-by: Mateo Fernandez <[email protected]> --------- Signed-off-by: Mateo Fernandez <[email protected]>
virt-do · May 6, 2024 · 1b67058 · 1b67058
1 parent 8ea1568
commit 1b67058
Show file tree

Hide file tree

Showing 7 changed files with 168 additions and 130 deletions.
diff --git a/README.md b/README.md
@@ -22,14 +22,14 @@
 Install the dependencies. On Debian/Ubuntu:
 
 ```bash
-apt install build-essential cmake pkg-config libssl-dev flex bison libelf-dev
+apt install build-essential cmake pkg-config libssl-dev flex bison libelf-dev iptables
 ```
 
-Then, configure the Rust toolchain and install [Just](https://github.com/casey/just):
+Then, configure the Rust toolchain and install [Just](https://github.com/casey/just) (only for dev environment):
 
 ```bash
 rustup target add x86_64-unknown-linux-musl
-cargo install just
+cargo install just # optional
 ```
 
 Finally, install [the protobuf compiler](https://github.com/protocolbuffers/protobuf?tab=readme-ov-file#protobuf-compiler-installation).

diff --git a/src/vmm/src/core/devices/virtio/net/bridge.rs b/src/vmm/src/core/devices/virtio/net/bridge.rs
@@ -1,9 +1,24 @@
-use std::net::{IpAddr, Ipv4Addr};
-
-use futures::stream::TryStreamExt;
-use rtnetlink::{new_connection, Error, Handle};
-
 use super::xx_netmask_width;
+use futures::stream::TryStreamExt;
+use rtnetlink::{new_connection, Handle};
+use std::net::{IpAddr, Ipv4Addr};
+use tracing::info;
+
+#[derive(Debug)]
+pub enum Error {
+    GetIndexByName {
+        name: String,
+        cause: rtnetlink::Error,
+    },
+    CreateBridge(rtnetlink::Error),
+    NoLinkPresent(String),
+    AddAddress(rtnetlink::Error),
+    AttachLink {
+        link_name: String,
+        cause: rtnetlink::Error,
+    },
+    SetStateAsUp(rtnetlink::Error),
+}
 
 #[derive(Clone)]
 pub struct Bridge {
@@ -12,126 +27,116 @@ pub struct Bridge {
 }
 
 impl Bridge {
-    pub fn new(name: String) -> Self {
+    pub async fn new(name: &str) -> Result<Self, Error> {
         let (connection, handle, _) = new_connection().unwrap();
         tokio::spawn(connection);
 
-        let br = Self { name, handle };
-        br.create_bridge_if_not_exist();
+        let br = Self {
+            name: name.into(),
+            handle,
+        };
+        br.create_bridge_if_not_exist().await?;
 
-        br
+        Ok(br)
     }
 
-    fn create_bridge_if_not_exist(&self) {
-        futures::executor::block_on(async {
-            let mut bridge_names = self
-                .handle
-                .link()
-                .get()
-                .match_name(self.name.clone())
-                .execute();
-
-            let _ = match bridge_names.try_next().await {
-                Ok(_) => Ok(()),
-                Err(_) => self
-                    .handle
-                    .link()
-                    .add()
-                    .bridge(self.name.clone())
-                    .execute()
-                    .await
-                    .map_err(|_| Error::RequestFailed),
-            };
-        });
+    async fn get_index_by_name(&self, name: &str) -> Result<u32, Error> {
+        let option = self
+            .handle
+            .link()
+            .get()
+            .match_name(name.into())
+            .execute()
+            .try_next()
+            .await
+            .map_err(|err| Error::GetIndexByName {
+                name: name.into(),
+                cause: err,
+            })?;
+
+        match option {
+            Some(a) => Ok(a.header.index),
+            None => Err(Error::NoLinkPresent(name.into())),
+        }
     }
 
-    pub fn set_addr(&self, addr: Ipv4Addr, netmask: Ipv4Addr) {
-        futures::executor::block_on(async {
-            let mut bridge_names = self
-                .handle
-                .link()
-                .get()
-                .match_name(self.name.clone())
-                .execute();
-
-            let bridge_index = match bridge_names.try_next().await {
-                Ok(Some(link)) => link.header.index,
-                Ok(None) => panic!(),
-                Err(_) => panic!(),
-            };
-
-            let prefix_len = xx_netmask_width(netmask.octets());
-
-            let _ = self
-                .handle
-                .address()
-                .add(bridge_index, IpAddr::V4(addr), prefix_len)
-                .execute()
-                .await
-                .map_err(|_| Error::RequestFailed);
-        });
+    async fn create_bridge_if_not_exist(&self) -> Result<(), Error> {
+        let result = self.get_index_by_name(&self.name).await;
+        if result.is_ok() {
+            info!("bridge is already presents");
+
+            return Ok(());
+        }
+
+        info!("bridge not found, creating bridge...");
+
+        self.handle
+            .clone()
+            .link()
+            .add()
+            .bridge(self.name.clone())
+            .execute()
+            .await
+            .map_err(Error::CreateBridge)
     }
 
-    pub fn set_up(&self) {
-        futures::executor::block_on(async {
-            let mut bridge_names = self
-                .handle
-                .link()
-                .get()
-                .match_name(self.name.clone())
-                .execute();
-
-            let bridge_index = match bridge_names.try_next().await {
-                Ok(Some(link)) => link.header.index,
-                Ok(None) => panic!(),
-                Err(_) => panic!(),
-            };
-
-            let _ = self
-                .handle
-                .link()
-                .set(bridge_index)
-                .up()
-                .execute()
-                .await
-                .map_err(|_| Error::RequestFailed);
-        });
+    pub async fn set_addr(&self, addr: Ipv4Addr, netmask: Ipv4Addr) -> Result<(), Error> {
+        let bridge_index = self.get_index_by_name(&self.name).await?;
+        let prefix_len = xx_netmask_width(netmask.octets());
+
+        let addr_get_request = self
+            .handle
+            .address()
+            .get()
+            .set_link_index_filter(bridge_index)
+            .set_address_filter(IpAddr::V4(addr))
+            .set_prefix_length_filter(prefix_len)
+            .execute()
+            .try_next()
+            .await;
+        if let Ok(Some(_)) = addr_get_request {
+            info!("address {:?} already exists for bridge", addr);
+
+            return Ok(());
+        }
+
+        info!(
+            "addr not found, set addr {} with mask {} for bridge",
+            addr, netmask
+        );
+        self.handle
+            .address()
+            .add(bridge_index, IpAddr::V4(addr), prefix_len)
+            .execute()
+            .await
+            .map_err(Error::AddAddress)
     }
 
-    pub fn attach_link(&self, link_name: String) {
-        futures::executor::block_on(async {
-            let mut link_names = self
-                .handle
-                .link()
-                .get()
-                .match_name(link_name.clone())
-                .execute();
-            let mut master_names = self
-                .handle
-                .link()
-                .get()
-                .match_name(self.name.clone())
-                .execute();
-
-            let link_index = match link_names.try_next().await {
-                Ok(Some(link)) => link.header.index,
-                Ok(None) => panic!(),
-                Err(_) => panic!(),
-            };
-            let master_index = match master_names.try_next().await {
-                Ok(Some(link)) => link.header.index,
-                Ok(None) => panic!(),
-                Err(_) => panic!(),
-            };
-
-            let _ = self
-                .handle
-                .link()
-                .set(link_index)
-                .controller(master_index)
-                .execute()
-                .await
-                .map_err(|_| Error::RequestFailed);
-        });
+    pub async fn attach_link(&self, link_name: String) -> Result<(), Error> {
+        let link_index = self.get_index_by_name(&link_name).await?;
+        let master_index = self.get_index_by_name(&self.name).await?;
+
+        self.handle
+            .link()
+            .set(link_index)
+            .controller(master_index)
+            .execute()
+            .await
+            .map_err(|err| Error::AttachLink {
+                link_name,
+                cause: err,
+            })
+    }
+
+    pub async fn set_up(&self) -> Result<(), Error> {
+        let bridge_index = self.get_index_by_name(&self.name).await?;
+
+        self.handle
+            .link()
+            .set(bridge_index)
+            .up()
+            .execute()
+            .await
+            .map_err(Error::SetStateAsUp)
     }
 }
diff --git a/src/vmm/src/core/devices/virtio/net/device.rs b/src/vmm/src/core/devices/virtio/net/device.rs
@@ -7,6 +7,7 @@ use super::{
 };
 use crate::core::devices::virtio::features::VIRTIO_F_RING_EVENT_IDX;
 use crate::core::devices::virtio::net::tuntap::open_tap::open_tap;
+use crate::core::devices::virtio::net::BRIDGE_NAME;
 use crate::core::devices::virtio::register::register_mmio_device;
 use crate::core::devices::virtio::{
     self, Config, MmioConfig, SingleFdSignalQueue, Subscriber, QUEUE_MAX_SIZE,
@@ -18,6 +19,7 @@ use std::{
     borrow::{Borrow, BorrowMut},
     sync::{Arc, Mutex},
 };
+use tracing::info;
 use virtio_bindings::{
     virtio_config::{VIRTIO_F_IN_ORDER, VIRTIO_F_VERSION_1},
     virtio_net::{
@@ -41,7 +43,7 @@ pub struct Net {
 
 impl Net {
     #[allow(clippy::too_many_arguments)]
-    pub fn new(
+    pub async fn new(
         mem: Arc<GuestMemoryMmap>,
         device_mgr: Arc<Mutex<IoManager>>,
         mmio_cfg: MmioConfig,
@@ -84,14 +86,29 @@ impl Net {
         tap.set_vnet_hdr_size(VIRTIO_NET_HDR_SIZE as i32)
             .map_err(Error::Tap)?;
 
-        let bridge_name = "br0".to_string();
-        let bridge = Bridge::new(bridge_name.clone());
-        bridge.set_addr(iface_host_addr, netmask);
-        bridge.attach_link(tap.get_name().map_err(Error::Tap)?);
-        bridge.set_up();
+        let bridge_name = BRIDGE_NAME;
+        let bridge = Bridge::new(bridge_name).await.map_err(Error::Bridge)?;
+
+        bridge
+            .set_addr(iface_host_addr, netmask)
+            .await
+            .map_err(Error::Bridge)?;
+
+        bridge
+            .attach_link(tap.get_name().map_err(Error::Tap)?)
+            .await
+            .map_err(Error::Bridge)?;
+        info!(
+            "attached link {:?} to bridge {}",
+            tap.get_name(),
+            bridge_name
+        );
+
+        bridge.set_up().await.map_err(Error::Bridge)?;
+        info!("bridge {} set UP", bridge_name);
 
         // Get internet access
-        iptables_ip_masq(iface_host_addr & netmask, netmask, bridge_name);
+        iptables_ip_masq(iface_host_addr & netmask, netmask, bridge_name.into());
 
         let net = Arc::new(Mutex::new(Net {
             mem,

diff --git a/src/vmm/src/core/devices/virtio/net/mod.rs b/src/vmm/src/core/devices/virtio/net/mod.rs
@@ -13,12 +13,14 @@ const NET_DEVICE_ID: u32 = 1;
 const VIRTIO_NET_HDR_SIZE: usize = 12;
 const RXQ_INDEX: u16 = 0;
 const TXQ_INDEX: u16 = 1;
+const BRIDGE_NAME: &str = "br0";
 
 #[derive(Debug)]
 pub enum Error {
     Virtio(virtio::Error),
     TunTap(open_tap::Error),
     Tap(tap::Error),
+    Bridge(bridge::Error),
 }
 
 pub type Result<T> = std::result::Result<T, Error>;