chore: revisit TODOs

src/acpi-tables/src/aml.rs

@@ -160,8 +160,6 @@ pub struct Name {
 
 impl Aml for Name {
     fn append_aml_bytes(&self, bytes: &mut Vec<u8>) -> Result<(), AmlError> {
-        // TODO: Refactor this to make more efficient but there are
-        // lifetime/ownership challenges.
         bytes.extend_from_slice(&self.bytes);
         Ok(())
     }

src/firecracker/src/metrics.rs

@@ -60,8 +60,6 @@ impl MutEventSubscriber for PeriodicMetrics {
         let source = event.fd();
         let event_set = event.event_set();
 
-        // TODO: also check for errors. Pending high level discussions on how we want
-        // to handle errors in devices.
         let supported_events = EventSet::IN;
         if !supported_events.contains(event_set) {
             warn!(

src/vmm/src/arch/aarch64/vcpu.rs

@@ -501,8 +501,6 @@ impl Peripherals {
     /// Returns error or enum specifying whether emulation was handled or interrupted.
     pub fn run_arch_emulation(&self, exit: VcpuExit) -> Result<VcpuEmulation, VcpuError> {
         METRICS.vcpu.failures.inc();
-        // TODO: Are we sure we want to finish running a vcpu upon
-        // receiving a vm exit that is not necessarily an error?
         error!("Unexpected exit reason on vcpu run: {:?}", exit);
         Err(VcpuError::UnhandledKvmExit(format!("{:?}", exit)))
     }

src/vmm/src/arch/x86_64/regs.rs

@@ -361,9 +361,8 @@ mod tests {
             ..Default::default()
         };
         let actual_fpu: kvm_fpu = vcpu.get_fpu().unwrap();
-        // TODO: auto-generate kvm related structures with PartialEq on.
         assert_eq!(expected_fpu.fcw, actual_fpu.fcw);
-        // Setting the mxcsr register from kvm_fpu inside setup_fpu does not influence anything.
+        // TODO: Setting the mxcsr register from kvm_fpu inside setup_fpu does not influence anything.
         // See 'kvm_arch_vcpu_ioctl_set_fpu' from arch/x86/kvm/x86.c.
         // The mxcsr will stay 0 and the assert below fails. Decide whether or not we should
         // remove it at all.

src/vmm/src/arch/x86_64/vcpu.rs

@@ -243,7 +243,7 @@ impl KvmVcpu {
         let extra_msrs = cpuid::common::msrs_to_save_by_cpuid(&kvm_cpuid);
         self.msrs_to_save.extend(extra_msrs);
 
-        // TODO: Some MSRs depend on values of other MSRs. This dependency will need to
+        // NOTE: Some MSRs depend on values of other MSRs. This dependency will need to
         // be implemented.
 
         // By this point we know that at snapshot, the list of MSRs we need to
@@ -586,9 +586,6 @@ impl KvmVcpu {
             .map_err(KvmVcpuError::VcpuGetDebugRegs)?;
         let lapic = self.fd.get_lapic().map_err(KvmVcpuError::VcpuGetLapic)?;
         let tsc_khz = self.get_tsc_khz().ok().or_else(|| {
-            // v0.25 and newer snapshots without TSC will only work on
-            // the same CPU model as the host on which they were taken.
-            // TODO: Add negative test for this warning failure.
             warn!("TSC freq not available. Snapshot cannot be loaded on a different CPU model.");
             None
         });

src/vmm/src/arch/x86_64/vm.rs

@@ -226,9 +226,7 @@ pub struct VmState {
     pub resource_allocator: ResourceAllocator,
     pitstate: kvm_pit_state2,
     clock: kvm_clock_data,
-    // TODO: rename this field to adopt inclusive language once Linux updates it, too.
     pic_master: kvm_irqchip,
-    // TODO: rename this field to adopt inclusive language once Linux updates it, too.
     pic_slave: kvm_irqchip,
     ioapic: kvm_irqchip,
 }

src/vmm/src/cpu_config/aarch64/custom_cpu_template.rs

@@ -22,7 +22,6 @@ impl GetCpuTemplate for Option<CpuTemplateType> {
             Some(template_type) => match template_type {
                 CpuTemplateType::Custom(template) => Ok(Cow::Borrowed(template)),
                 CpuTemplateType::Static(template) => match template {
-                    // TODO: Check if the CPU model is Neoverse-V1.
                     StaticCpuTemplate::V1N1 => Ok(Cow::Owned(v1n1::v1n1())),
                     other => Err(GetCpuTemplateError::InvalidStaticCpuTemplate(*other)),
                 },

src/vmm/src/cpu_config/x86_64/cpuid/intel/normalize.rs

@@ -312,8 +312,6 @@ pub enum DefaultBrandStringError {
 ///
 /// Never.
 // As we pass through host frequency, we require CPUID and thus `cfg(cpuid)`.
-// TODO: Use `split_array_ref`
-// (https://github.com/firecracker-microvm/firecracker/issues/3347)
 #[allow(clippy::indexing_slicing, clippy::arithmetic_side_effects)]
 #[inline]
 fn default_brand_string(

src/vmm/src/dumbo/pdu/ethernet.rs

@@ -15,8 +15,6 @@ const SRC_MAC_OFFSET: usize = 6;
 const ETHERTYPE_OFFSET: usize = 12;
 
 // We don't support 802.1Q tags.
-// TODO: support 802.1Q tags?! If so, don't forget to change the speculative_test_* functions
-// for ARP and IPv4.
 /// Payload offset in an ethernet frame
 pub const PAYLOAD_OFFSET: usize = 14;
 

src/vmm/src/dumbo/pdu/tcp.rs

@@ -88,9 +88,6 @@ pub enum TcpError {
     SliceTooShort,
 }
 
-// TODO: The implementation of TcpSegment is IPv4 specific in regard to checksum computation. Maybe
-// make it more generic at some point.
-
 /// Interprets the inner bytes as a TCP segment.
 #[derive(Debug)]
 pub struct TcpSegment<'a, T: 'a> {

src/vmm/src/dumbo/tcp/connection.rs

@@ -241,9 +241,6 @@ impl Connection {
             return Err(PassiveOpenError::InvalidSyn);
         }
 
-        // TODO: If we ever implement window scaling, change the part that computes
-        // remote_rwnd_edge below.
-
         // We only care about the MSS option for now.
         let mss = parse_mss_option(segment)?;
 
@@ -528,16 +525,10 @@ impl Connection {
             return Err(RecvError::ConnectionReset);
         }
 
-        // TODO: The following logic fully makes sense only for a passive open (which is what we
-        // currently support). Things must change a bit if/when we also implement active opens.
-
         let segment_flags = s.flags_after_ns();
-
         if segment_flags.intersects(TcpFlags::RST) {
             let seq = Wrapping(s.sequence_number());
             // We accept the RST only if it carries an in-window sequence number.
-            // TODO: If/when we support active opens, we'll also have to accept RST/SYN segments,
-            // which must acknowledge our SYN to be valid.
             if seq_at_or_after(seq, self.ack_to_send) && seq_after(self.local_rwnd_edge, seq) {
                 self.set_flags(ConnStatusFlags::RESET);
                 return Ok((None, RecvStatusFlags::RESET_RECEIVED));
@@ -575,7 +566,6 @@ impl Connection {
             // Reaching this branch means the connection is ESTABLISHED. The only thing we want to
             // do right now is reset if we get segments which carry the SYN flag, because they are
             // obviously invalid, and something must be really wrong.
-            // TODO: Is it an overreaction to reset here?
             if s.flags_after_ns().intersects(TcpFlags::SYN) {
                 return self.reset_for_segment_helper(s, RecvStatusFlags::INVALID_SEGMENT);
             }
@@ -657,9 +647,8 @@ impl Connection {
             if let Some(fin_seq) = self.fin_received
                 && !seq_at_or_after(fin_seq, data_end_seq)
             {
-                // TODO: This is a strange situation, because the other endpoint is sending data
-                // after it initially closed its half of the connection. We simply ignore the
-                // segment for now.
+                // In a situation, when other endpoint is sending data after it initially closed
+                // its half of the connection, we simply ignore the segment.
                 return Ok((None, recv_status_flags | RecvStatusFlags::DATA_BEYOND_FIN));
             }
 
@@ -676,9 +665,6 @@ impl Connection {
             // We currently assume segments are seldom lost or reordered, and only accept those with
             // the exact next sequence number we're waiting for.
             if seq != self.ack_to_send {
-                // TODO: Maybe we should enqueue multiple ACKs here (after making such a thing
-                // possible in the first place), just so we're more likely to trigger a
-                // retransmission.
                 self.enqueue_ack();
                 return Ok((None, recv_status_flags | RecvStatusFlags::UNEXPECTED_SEQ));
             }
@@ -826,10 +812,6 @@ impl Connection {
         payload_src: PayloadSource<R>,
         now: u64,
     ) -> Result<Option<Incomplete<TcpSegment<'a, &'a mut [u8]>>>, WriteNextError> {
-        // TODO: like receive_segment(), this function is specific in some ways to Connections
-        // created via passive open. When/if we also implement active opens, some things will
-        // have to change.
-
         if self.is_reset() {
             return Err(WriteNextError::ConnectionReset);
         }
@@ -1012,10 +994,6 @@ impl Connection {
     }
 }
 
-// TODO: I'll be honest: the tests here cover the situations most likely to be encountered, but are
-// not even close to being exhaustive. Something like that might be worth pursuing after polishing
-// the rougher edges around the current implementation, and deciding its scope relative to an
-// actual TCP implementation.
 #[cfg(test)]
 pub(crate) mod tests {
     use super::*;

src/vmm/src/dumbo/tcp/endpoint.rs

@@ -24,7 +24,7 @@ use crate::dumbo::tcp::connection::{Connection, PassiveOpenError, RecvStatusFlag
 use crate::dumbo::tcp::{MAX_WINDOW_SIZE, NextSegmentStatus, seq_after};
 use crate::logger::{IncMetric, METRICS};
 
-// TODO: These are currently expressed in cycles. Normally, they would be the equivalent of a
+// These are currently expressed in cycles. Normally, they would be the equivalent of a
 // certain duration, depending on the frequency of the CPU, but we still have a bit to go until
 // that functionality is available, so we just use some conservative-ish values. Even on a fast
 // 4GHz CPU, the first is roughly equal to 10 seconds, and the other is ~300 ms.
@@ -115,9 +115,6 @@ impl Endpoint {
             receive_buf: [0u8; RCV_BUF_MAX_SIZE as usize],
             receive_buf_left: 0,
             response_buf: Vec::new(),
-            // TODO: Using first_not_sent() makes sense here because a connection is currently
-            // created via passive open only, so this points to the sequence number right after
-            // the SYNACK. It might stop working like that if/when the implementation changes.
             response_seq: connection.first_not_sent(),
             initial_response_seq: connection.first_not_sent(),
             connection,

src/vmm/src/dumbo/tcp/handler.rs

@@ -18,8 +18,6 @@ use crate::dumbo::pdu::tcp::{Flags as TcpFlags, TcpError as TcpSegmentError, Tcp
 use crate::dumbo::tcp::endpoint::Endpoint;
 use crate::dumbo::tcp::{NextSegmentStatus, RstConfig};
 
-// TODO: This is currently IPv4 specific. Maybe change it to a more generic implementation.
-
 /// Describes events which may occur when the handler receives packets.
 #[derive(Debug, PartialEq, Eq)]
 pub enum RecvEvent {
@@ -349,7 +347,6 @@ impl TcpIPv4Handler {
         }
     }
 
-    // TODO: I guess this should be refactored at some point to also remove the endpoint if found.
     fn find_evictable_connection(&self) -> Option<ConnectionTuple> {
         for (tuple, endpoint) in self.connections.iter() {
             if endpoint.is_evictable() {

src/vmm/src/mmds/ns.rs

@@ -1,7 +1,6 @@
 // Copyright 2018 Amazon.com, Inc. or its affiliates. All Rights Reserved.
 // SPDX-License-Identifier: Apache-2.0
 
-// TODO: get rid of this when splitting dumbo into public and internal parts.
 #![allow(missing_docs)]
 
 use std::convert::From;

src/vmm/src/rate_limiter/mod.rs

@@ -475,7 +475,7 @@ impl RateLimiter {
     }
 
     /// Updates the parameters of the token buckets associated with this RateLimiter.
-    // TODO: Please note that, right now, the buckets become full after being updated.
+    /// Buckets become full after the update.
     pub fn update_buckets(&mut self, bytes: BucketUpdate, ops: BucketUpdate) {
         match bytes {
             BucketUpdate::Disabled => self.bandwidth = None,

src/vmm/src/vmm_config/mod.rs

@@ -36,15 +36,6 @@ pub mod snapshot;
 /// Wrapper for configuring the vsock devices attached to the microVM.
 pub mod vsock;
 
-// TODO: Migrate the VMM public-facing code (i.e. interface) to use stateless structures,
-// for receiving data/args, such as the below `RateLimiterConfig` and `TokenBucketConfig`.
-// Also todo: find a better suffix than `Config`; it should illustrate the static nature
-// of the enclosed data.
-// Currently, data is passed around using live/stateful objects. Switching to static/stateless
-// objects will simplify both the ownership model and serialization.
-// Public access would then be more tightly regulated via `VmmAction`s, consisting of tuples like
-// (entry-point-into-VMM-logic, stateless-args-structure).
-
 /// A public-facing, stateless structure, holding all the data we need to create a TokenBucket
 /// (live) object.
 #[derive(Clone, Copy, Debug, Default, PartialEq, Eq, Deserialize, Serialize)]