Browse Source

Address code review suggestions

- Handle payload copy outside of mld::AddressRecordRepr::emit()
- On unit test, use the parsed Hop-by-Hop option instead of checking
  that the wire representation of the two structures is the same
- Minor code changes
Lucas Villa Real 1 năm trước cách đây
mục cha
commit
db25ee4384
5 tập tin đã thay đổi với 41 bổ sung40 xóa
  1. 2 2
      ci.sh
  2. 17 17
      src/iface/interface/ipv6.rs
  3. 16 16
      src/iface/interface/tests/ipv6.rs
  4. 2 0
      src/socket/dns.rs
  5. 4 5
      src/wire/mld.rs

+ 2 - 2
ci.sh

@@ -21,7 +21,7 @@ FEATURES_TEST=(
     "std,medium-ethernet,proto-ipv4,proto-igmp,socket-raw,socket-dns"
     "std,medium-ethernet,proto-ipv4,socket-udp,socket-tcp,socket-dns"
     "std,medium-ethernet,proto-ipv4,proto-dhcpv4,socket-udp"
-    "std,medium-ethernet,medium-ip,medium-ieee802154,proto-ipv6,proto-igmp,socket-udp,socket-dns"
+    "std,medium-ethernet,medium-ip,medium-ieee802154,proto-ipv6,proto-igmp,proto-rpl,socket-udp,socket-dns"
     "std,medium-ethernet,proto-ipv6,socket-tcp"
     "std,medium-ethernet,medium-ip,proto-ipv4,socket-icmp,socket-tcp"
     "std,medium-ip,proto-ipv6,socket-icmp,socket-tcp"
@@ -29,7 +29,7 @@ FEATURES_TEST=(
     "std,medium-ieee802154,proto-sixlowpan,proto-sixlowpan-fragmentation,socket-udp"
     "std,medium-ieee802154,proto-rpl,proto-sixlowpan,proto-sixlowpan-fragmentation,socket-udp"
     "std,medium-ip,proto-ipv4,proto-ipv6,socket-tcp,socket-udp"
-    "std,medium-ethernet,medium-ip,medium-ieee802154,proto-ipv4,proto-ipv6,proto-igmp,socket-raw,socket-udp,socket-tcp,socket-icmp,socket-dns,async"
+    "std,medium-ethernet,medium-ip,medium-ieee802154,proto-ipv4,proto-ipv6,proto-igmp,proto-rpl,socket-raw,socket-udp,socket-tcp,socket-icmp,socket-dns,async"
     "std,medium-ieee802154,medium-ip,proto-ipv4,socket-raw"
     "std,medium-ethernet,proto-ipv4,proto-ipsec,socket-raw"
 )

+ 17 - 17
src/iface/interface/ipv6.rs

@@ -166,6 +166,23 @@ impl InterfaceInner {
         })
     }
 
+    /// Get the first link-local IPv6 address of the interface, if present.
+    fn link_local_ipv6_address(&self) -> Option<Ipv6Address> {
+        self.ip_addrs.iter().find_map(|addr| match *addr {
+            #[cfg(feature = "proto-ipv4")]
+            IpCidr::Ipv4(_) => None,
+            #[cfg(feature = "proto-ipv6")]
+            IpCidr::Ipv6(cidr) => {
+                let addr = cidr.address();
+                if addr.is_link_local() {
+                    Some(addr)
+                } else {
+                    None
+                }
+            }
+        })
+    }
+
     pub(super) fn process_ipv6<'frame>(
         &mut self,
         sockets: &mut SocketSet,
@@ -519,21 +536,4 @@ impl InterfaceInner {
             IpPayload::HopByHopIcmpv6(hbh_repr, Icmpv6Repr::Mld(mld_repr)),
         ))
     }
-
-    /// Get the first link-local IPv6 address of the interface, if present.
-    fn link_local_ipv6_address(&self) -> Option<Ipv6Address> {
-        self.ip_addrs.iter().find_map(|addr| match *addr {
-            #[cfg(feature = "proto-ipv4")]
-            IpCidr::Ipv4(_) => None,
-            #[cfg(feature = "proto-ipv6")]
-            IpCidr::Ipv6(cidr) => {
-                let addr = cidr.address();
-                if addr.is_link_local() {
-                    Some(addr)
-                } else {
-                    None
-                }
-            }
-        })
-    }
 }

+ 16 - 16
src/iface/interface/tests/ipv6.rs

@@ -1231,25 +1231,25 @@ fn test_join_ipv6_multicast_group(#[case] medium: Medium) {
         let ip_payload = ipv6_packet.payload();
 
         // The first 2 octets of this payload hold the next-header indicator and the
-        // Hop-by-Hop header length (in 8-octet words, minus 1), which we parse as an
-        // Unknown option. The remaining 6 octets hold the Hop-by-Hop PadN and Router
-        // Alert options.
+        // Hop-by-Hop header length (in 8-octet words, minus 1). The remaining 6 octets
+        // hold the Hop-by-Hop PadN and Router Alert options.
         let hbh_header = Ipv6HopByHopHeader::new_checked(&ip_payload[..8]).unwrap();
         let hbh_repr = Ipv6HopByHopRepr::parse(&hbh_header).unwrap();
 
-        let mut expected_hbh_repr = Ipv6HopByHopRepr::mldv2_router_alert(0);
-        expected_hbh_repr
-            .options
-            .insert(
-                0,
-                Ipv6OptionRepr::Unknown {
-                    type_: Ipv6OptionType::Unknown(IpProtocol::Icmpv6.into()),
-                    length: 0,
-                    data: &[],
-                },
-            )
-            .unwrap();
-        assert_eq!(hbh_repr, expected_hbh_repr);
+        assert_eq!(hbh_repr.options.len(), 3);
+        assert_eq!(
+            hbh_repr.options[0],
+            Ipv6OptionRepr::Unknown {
+                type_: Ipv6OptionType::Unknown(IpProtocol::Icmpv6.into()),
+                length: 0,
+                data: &[],
+            }
+        );
+        assert_eq!(hbh_repr.options[1], Ipv6OptionRepr::PadN(0));
+        assert_eq!(
+            hbh_repr.options[2],
+            Ipv6OptionRepr::RouterAlert(Ipv6OptionRouterAlert::MulticastListenerDiscovery)
+        );
 
         let icmpv6_packet =
             Icmpv6Packet::new_checked(&ip_payload[hbh_repr.buffer_len()..]).unwrap();

+ 2 - 0
src/socket/dns.rs

@@ -20,11 +20,13 @@ const MAX_RETRANSMIT_DELAY: Duration = Duration::from_millis(10_000);
 const RETRANSMIT_TIMEOUT: Duration = Duration::from_millis(10_000); // Should generally be 2-10 secs
 
 #[cfg(feature = "proto-ipv6")]
+#[allow(unused)]
 const MDNS_IPV6_ADDR: IpAddress = IpAddress::Ipv6(crate::wire::Ipv6Address([
     0xff, 0x02, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0xfb,
 ]));
 
 #[cfg(feature = "proto-ipv4")]
+#[allow(unused)]
 const MDNS_IPV4_ADDR: IpAddress = IpAddress::Ipv4(crate::wire::Ipv4Address([224, 0, 0, 251]));
 
 /// Error returned by [`Socket::start_query`]

+ 4 - 5
src/wire/mld.rs

@@ -343,11 +343,6 @@ impl<'a> AddressRecordRepr<'a> {
         record.set_aux_data_len(self.aux_data_len);
         record.set_num_srcs(self.num_srcs);
         record.set_mcast_addr(self.mcast_addr);
-        let record_payload = record.payload_mut();
-        if self.payload.len() == record_payload.len() {
-            // TODO: handle the case where the payload sizes are different
-            record_payload.copy_from_slice(self.payload);
-        }
     }
 }
 
@@ -452,6 +447,10 @@ impl<'a> Repr<'a> {
                 packet.set_nr_mcast_addr_rcrds(records.len() as u16);
                 let mut payload = packet.payload_mut();
                 for record in *records {
+                    if record.payload.len() == payload.len() {
+                        // TODO: handle the case where the payload sizes are different
+                        payload.copy_from_slice(record.payload);
+                    }
                     record.emit(&mut AddressRecord::new_unchecked(&mut *payload));
                     payload = &mut payload[record.buffer_len()..];
                 }