Incorporate review comments

Signed-off-by: Guvenc Gulce <guevenc.guelce@sap.com>

Author: guvenc

include/dp_port.h

@@ -48,6 +48,7 @@ struct dp_port_iface {
 	bool					arp_done;
 	uint64_t				total_flow_rate_cap;
 	uint64_t				public_flow_rate_cap;
+	uint32_t				hostname_len;
 };
 
 struct dp_port_async_template {

include/nodes/dhcp_node.h

@@ -21,6 +21,7 @@ extern "C" {
 #define DHCP_OPT_SUBNET_MASK     1
 #define DHCP_OPT_ROUTER          3
 #define DHCP_OPT_DNS             6
+#define DHCP_OPT_HOSTNAME        12
 #define DHCP_OPT_INTERFACE_MTU   26
 #define DHCP_OPT_IP_LEASE_TIME   51
 #define DHCP_OPT_MESSAGE_TYPE    53

src/grpc/dp_grpc_impl.c

@@ -461,9 +461,8 @@ static int dp_process_create_interface(struct dp_grpc_responder *responder)
 	static_assert(sizeof(request->pxe_str) == sizeof(port->iface.cfg.pxe_str), "Incompatible interface PXE size");
 	rte_memcpy(port->iface.cfg.pxe_str, request->pxe_str, sizeof(port->iface.cfg.pxe_str));
 	dp_copy_ipaddr(&port->iface.cfg.pxe_ip, &request->pxe_addr);
-	// Ensure null termination even if source is exactly max length
-	rte_memcpy(port->iface.cfg.hostname, request->hostname, sizeof(port->iface.cfg.hostname) - 1);
-	port->iface.cfg.hostname[sizeof(port->iface.cfg.hostname) - 1] = '\0';
+	rte_memcpy(port->iface.cfg.hostname, request->hostname, sizeof(port->iface.cfg.hostname));
+	port->iface.hostname_len = (uint32_t)strnlen(port->iface.cfg.hostname, DP_IFACE_HOSTNAME_MAX_LEN - 1);
 
 	/* Do not install routes for an empty(zero) IP, as zero ip is just a marker for showing the disabled IPv4/IPv6 machinery */
 	if (request->ip4_addr != 0) {

src/nodes/dhcp_node.c

@@ -49,8 +49,6 @@ static uint8_t classless_route[sizeof(classless_route_prefix) + sizeof(server_ip
 static_assert(DP_VIRTSVC_MAX <= UINT8_MAX+1, "Number of virtual services can be higher than supported link-local subnet size");
 #endif
 
-#define DHCP_OPT_HOSTNAME 12
-
 static int dhcp_node_init(__rte_unused const struct rte_graph *graph, __rte_unused struct rte_node *node)
 {
 	server_ip = htonl(dp_get_gw_ip4());
@@ -89,11 +87,10 @@ static __rte_always_inline int add_dhcp_option(uint8_t **pos_ptr, uint8_t *end,
 static __rte_always_inline int add_dhcp_options(struct dp_dhcp_header *dhcp_hdr,
 												 uint8_t msg_type,
 												 enum dp_pxe_mode pxe_mode,
-												 const char *hostname)
+												 const struct dp_port *port)
 {
 	uint8_t *pos = dhcp_hdr->options;
 	uint8_t *end = pos + DHCP_MAX_OPTIONS_LEN;
-	uint8_t hostname_len;
 	const struct dp_conf_dhcp_dns *dhcp_dns = dp_conf_get_dhcp_dns();
 
 	if (DP_FAILED(add_dhcp_option(&pos, end, DHCP_OPT_MESSAGE_TYPE, &msg_type, sizeof(msg_type)))
@@ -113,12 +110,9 @@ static __rte_always_inline int add_dhcp_options(struct dp_dhcp_header *dhcp_hdr,
 		if (DP_FAILED(add_dhcp_option(&pos, end, DHCP_OPT_DNS, dhcp_dns->array, dhcp_dns->len)))
 			return DP_ERROR;
 
-	if (hostname && hostname[0] != '\0') {
-		hostname_len = (uint8_t)strnlen(hostname, DP_IFACE_HOSTNAME_MAX_LEN - 1);
-
-		if (DP_FAILED(add_dhcp_option(&pos, end, DHCP_OPT_HOSTNAME, hostname, hostname_len)))
+	if (port->iface.hostname_len > 0)
+		if (DP_FAILED(add_dhcp_option(&pos, end, DHCP_OPT_HOSTNAME, port->iface.cfg.hostname, (u_int8_t)port->iface.hostname_len)))
 			return DP_ERROR;
-	}
 
 	if (pos >= end)
 		return DP_ERROR;
@@ -190,7 +184,6 @@ static __rte_always_inline rte_edge_t get_next_index(struct rte_node *node, stru
 	rte_be32_t pxe_srv_ip;
 	char pxe_srv_ip_str[INET6_ADDRSTRLEN];
 	uint8_t response_type;
-	const char *hostname;
 
 	incoming_eth_hdr = rte_pktmbuf_mtod(m, struct rte_ether_hdr *);
 	incoming_ipv4_hdr = (struct rte_ipv4_hdr *)(incoming_eth_hdr + 1);
@@ -274,8 +267,7 @@ static __rte_always_inline rte_edge_t get_next_index(struct rte_node *node, stru
 		dhcp_hdr->siaddr = server_ip;
 	}
 
-	hostname = port->iface.cfg.hostname;
-	options_len = add_dhcp_options(dhcp_hdr, response_type, pxe_mode, hostname);
+	options_len = add_dhcp_options(dhcp_hdr, response_type, pxe_mode, port);
 	if (DP_FAILED(options_len)) {
 		DPNODE_LOG_WARNING(node, "DHCP response options too large for a packet");
 		return DHCP_NEXT_DROP;

test/local/config.py

@@ -132,13 +132,17 @@ def create(vni):
 		vm.ip = f"{ov_ip_prefix}{vni}.1.{VMSpec._idx+1}"
 		vm.ipv6 = f"{ov_ipv6_prefix}{vni}:1::{VMSpec._idx+1}"
 		vm.ul_ipv6 = None  # will be assigned dynamically
+		vm.hostname = None
 		VMSpec._idx += 1
 		return vm
+	def set_hostname(self, hostname):
+		self.hostname = hostname
 
 PF0 = PFSpec.create()
 PF1 = PFSpec.create()
 # VM1 and VM2 are on the same VNI
 VM1 = VMSpec.create(vni1)
+VM1.set_hostname("vm1-host")
 VM2 = VMSpec.create(vni1)
 # VM3 is on the second VNI
 VM3 = VMSpec.create(vni2)

test/local/dp_service.py

@@ -86,9 +86,9 @@ def init_ifaces(self, grpc_client):
 		if not self.hardware:  # see above
 			interface_init(PF1.tap, self.port_redundancy)
 		grpc_client.init()
-		VM1.ul_ipv6 = grpc_client.addinterface(VM1.name, VM1.pci, VM1.vni, VM1.ip, VM1.ipv6, pxe_server, ipxe_file_name, hostname=VM1.name)
+		VM1.ul_ipv6 = grpc_client.addinterface(VM1.name, VM1.pci, VM1.vni, VM1.ip, VM1.ipv6, pxe_server, ipxe_file_name, hostname=VM1.hostname)
 		VM2.ul_ipv6 = grpc_client.addinterface(VM2.name, VM2.pci, VM2.vni, VM2.ip, VM2.ipv6, pxe_server, ipxe_file_name)
-		VM3.ul_ipv6 = grpc_client.addinterface(VM3.name, VM3.pci, VM3.vni, VM3.ip, VM3.ipv6, hostname=VM3.name)
+		VM3.ul_ipv6 = grpc_client.addinterface(VM3.name, VM3.pci, VM3.vni, VM3.ip, VM3.ipv6)
 		grpc_client.addroute(vni1, neigh_vni1_ov_ip_route, 0, neigh_vni1_ul_ipv6)
 		grpc_client.addroute(vni1, neigh_vni1_ov_ipv6_route, 0, neigh_vni1_ul_ipv6)
 		grpc_client.addroute(vni1, "0.0.0.0/0", vni1, router_ul_ipv6)

test/local/helpers.py

@@ -12,7 +12,7 @@
 from config import *
 
 
-def request_ip(vm, check_hostname=False):
+def request_ip(vm):
 	scapy.config.conf.checkIPaddr = False
 	answer = dhcp_request(iface=vm.tap, timeout=sniff_timeout)
 	validate_checksums(answer)
@@ -27,11 +27,10 @@ def request_ip(vm, check_hostname=False):
 	if not dns_servers or dhcp_dns1 not in dns_servers or dhcp_dns2 not in dns_servers:
 		raise AssertionError(f"DHCP message does not specify the right DNS servers: {dns_servers} instead of {dhcp_dns1} and {dhcp_dns2}")
 
-	# Check for hostname option
-	if check_hostname:
+	if vm.hostname != None:
 		hostname_option = next((opt[1] for opt in options if opt[0] == 'hostname'), None)
 		assert hostname_option is not None, "Hostname option not in DHCP reply"
-		expected_hostname = vm.name
+		expected_hostname = vm.hostname
 		assert hostname_option.decode('utf-8') == expected_hostname, \
 			f"DHCP reply does not specify the correct hostname: {hostname_option.decode('utf-8')} instead of {expected_hostname}"
 

test/local/test_dhcpv4.py

@@ -4,7 +4,7 @@
 from helpers import *
 
 def test_dhcpv4_vf0(prepare_ifaces):
-	request_ip(VM1, check_hostname=True)
+	request_ip(VM1)
 
 def test_dhcpv4_vf1(prepare_ifaces):
 	request_ip(VM2)