address review comments to enhance code

docs/deployment/capture_offloaded_rx_pkts.md

@@ -1,5 +1,5 @@
 # Feature: capture offloaded rx packets on interfaces
-In offloaded mode, packets that are processed by hardware offloed rules cannot be seen anymore even on the software path. To increase the visibility of this type of traffic flows, we use special rte flow rules to instrument packet processing on hardware to duplicate and capture these packets on interfaces.
+In offloaded mode, packets that are processed by hardware offload rules cannot be seen anymore even on the software path. To increase the visibility of this type of traffic flows, we use special rte flow rules to instrument packet processing on hardware to duplicate and capture these packets on interfaces.
 
 ## What can be achieved and what cannot
 Through tedious and complex experiment, the following features are identified and thus currently supported:
@@ -19,33 +19,38 @@ Capturing must be started via dpservice-cli before the first packets of new flow
 
 
 ```
-./bin/dpservice-cli capture start --sink-node-ip=<underly IP of the hypervisor> --udp-src-port=<selected port ID> --udp-dst-port=<selected port ID>  --vf=<list of started interfaces> --pf=0
+./bin/dpservice-cli capture start --sink-node-ip=<underlay IP of the hypervisor or a remote host> --udp-src-port=<selected port ID> --udp-dst-port=<selected port ID>  --vf=<list of started interfaces> --pf=0
 ```
 
 for example:
 ```
 ./bin/dpservice-cli capture start --sink-node-ip=abcd:efgh:1234:4321::1 --udp-src-port=3000 --udp-dst-port=3010  --vf=vm-1,vm-2 --pf=0
 ```
 
-The captured packets will be transmitted back to the hypervisors interface (via router) in an encapped format, which is visible on pf0 interface using a regular tcpdump tool. For example, these packets can be dumped to a pcap file using a command:
+The captured packets will be transmitted back in an encapped format to the interface (via router) of your selected sink machine, either the hypervisor where dp-service is running or a remote host. These packets are visible on physical interfaces using a regular tcpdump tool. For example, these packets can be dumped to a pcap file using a command:
 
 ```
 sudo tcpdump -ni any udp dst port 3010 -w test.pcap
 ```
 
-The generated test.pcap file can be opened using Wireshark(graphic). As captured packets are encaped as UDP payload, this file can be firstly modified by removingß the first 62 bytes of all packets.
+The generated test.pcap file can be opened using Wireshark(graphic). As captured packets are encaped as UDP payload, this file can be firstly modified by removing the first 62 bytes of all packets.
 
 ```
 editcap -C 62 -F pcap test.pcap test_no_udp.pcap
 ```
 
 The resulted test_no_udp.pcap file can be recognized by wireshark.
 
+The following command is used to stop capturing on all configured interfaces. Note that, to start capturing on a new set of interfaces, this stopping command has to be called first.
+```
+/bin/dpservice-cli capture stop
+```
+
 ## How offloaded packets are captured
-Offloaded packets are captured by using special rte flow rules, especial the one that enables pkt sampling on the RX side of an interface. The captured packets are encapsulated by prepending extra headers. Despite of the fact that captured Ethernet frames are treated as UDP payload, it is flexible to use other customized headers as well. The format of encapsulation is as following:
+Offloaded packets are captured by using special rte flow rules, especially the one that enables packet sampling on the RX side of an interface. The captured packets are encapsulated by prepending extra headers. Despite the fact that captured Ethernet frames are treated as UDP payload, it is flexible to use other customized headers as well. The format of encapsulation is as follows:
 
 ```
-| Outter Ether header | Outter IPv6 header | UDP header | Captured Ether frame |
+| Outer Ether header | Outer IPv6 header | UDP header | Captured Ether frame |
 ```
 
 [Figure1](docs/sys_design/pkt_capture_flow_rules-VF.drawio.png) and [Figure2](docs/sys_design/pkt_capture_flow_rules-PF.drawio.png) illustrate the organization of flow rules for VF and PF. The differences between handling VF and PF are empirical.

include/dp_error.h

@@ -61,16 +61,14 @@ const char *dp_strerror_verbose(int error);
 	ERR(NO_BACKIP,							421) \
 	ERR(NO_LB,								422) \
 	ERR(NO_DROP_SUPPORT,					441) \
-	ERR(CAPTURE_CANNOT_INIT,				451) \
-	ERR(CAPTURE_INIT_CANNOT_ROLLBACK,		452) \
-	ERR(CAPTURE_INIT_INVALID_PORT_ID,		453) \
-	ERR(CAPTURE_INIT_PORT_NOT_ALLOC,		454) \
-	ERR(CAPTURE_INIT_FAILED_SET_PF,			455) \
-	ERR(CAPTURE_INIT_FAILED_SET_VF,			456) \
-	ERR(CAPTURE_INIT_PORT_ALREADY_SET,		457) \
-	ERR(CAPTURE_INIT_FAILED_DEL_DEFAULT,	458) \
-	ERR(CAPTURE_INIT_FAILED_RECOVER,		459) \
-	ERR(CAPTURE_CANNOT_STOP,				460) \
+	ERR(CAPTURE_CANNOT_ROLLBACK,		452) \
+	ERR(CAPTURE_INVALID_PORT_ID,		453) \
+	ERR(CAPTURE_PORT_NOT_ALLOC,		454) \
+	ERR(CAPTURE_FAILED_SET_PF,			455) \
+	ERR(CAPTURE_FAILED_SET_VF,			456) \
+	ERR(CAPTURE_ALREADY_SET,		457) \
+	ERR(CAPTURE_FAILED_DEL_DEFAULT,	458) \
+	ERR(CAPTURE_FAILED_RECOVER,		459) \
 
 #define _DP_GRPC_ERROR_ENUM(NAME, NUMBER) \
 	DP_GRPC_ERR_##NAME = _DP_GRPC_ERRCODES - NUMBER,

include/grpc/dp_grpc_api.h

@@ -173,22 +173,17 @@ struct dpgrpc_capture_interface {
 	union {
 		char	iface_id[VM_IFACE_ID_MAX_LEN];
 		uint8_t pf_index;
-	} interface_info;
+	} spec;
 };
 
-struct dpgrpc_capture_config {
+struct dpgrpc_capture {
 	uint8_t			dst_addr6[DP_VNF_IPV6_ADDR_SIZE];
-	uint8_t			filled_interface_info_count;
+	uint8_t			interface_count;
 	uint32_t		udp_src_port;
 	uint32_t		udp_dst_port;
 	struct dpgrpc_capture_interface interfaces[DP_CAPTURE_MAX_PORT_NUM];
 };
 
-struct dpgrpc_capture_stat {
-	uint8_t							status;
-	struct dpgrpc_capture_interface interface;
-};
-
 struct dpgrpc_capture_stop {
 	uint16_t						port_cnt;
 };
@@ -231,7 +226,7 @@ struct dpgrpc_request {
 		struct dpgrpc_vni		vni_in_use;
 		struct dpgrpc_vni		vni_reset;
 		struct dpgrpc_versions	get_version;
-		struct dpgrpc_capture_config	start_capture;
+		struct dpgrpc_capture	start_capture;
 	};
 };
 
@@ -269,7 +264,6 @@ struct dpgrpc_reply {
 		struct dpgrpc_fwrule_info	fwrule;
 		struct dpgrpc_vni_in_use	vni_in_use;
 		struct dpgrpc_versions		versions;
-		struct dpgrpc_capture_stat 	capture_stat;
 		struct dpgrpc_capture_stop	capture_stop;
 	};
 };

include/monitoring/dp_monitoring.h

@@ -31,15 +31,17 @@ struct dp_event_msg {
 	} event_entry;
 };
 
+struct dp_capture_hdr_config {
+	uint8_t capture_node_ipv6_addr[16];
+	uint32_t capture_udp_src_port;
+	uint32_t capture_udp_dst_port;
+};
+
 void dp_process_event_msg(struct rte_mbuf *m);
 
-void dp_set_capture_node_ipv6_addr(uint8_t *addr);
-void dp_set_capture_udp_src_port(uint32_t port);
 
-void dp_set_capture_udp_dst_port(uint32_t port);
-uint8_t *dp_get_capture_node_ipv6_addr(void);
-uint16_t dp_get_capture_udp_src_port(void);
-uint16_t dp_get_capture_udp_dst_port(void);
+void dp_set_capture_hdr_config(uint8_t *addr, uint32_t udp_src_port, uint32_t udp_dst_port);
+struct dp_capture_hdr_config *dp_get_capture_hdr_config(void);
 
 void dp_set_capture_enabled(bool enabled);
 

include/rte_flow/dp_rte_flow_init.h

@@ -13,18 +13,17 @@ extern "C" {
 int dp_install_isolated_mode_ipip(int port_id, uint8_t proto_id);
 
 int dp_install_jump_rule_in_default_group(uint16_t port_id, uint32_t group_id);
-int dp_install_default_rule_in_capture_group(uint16_t port_id, bool is_on);
+int dp_install_default_rule_in_capture_group(uint16_t port_id, bool capture_on);
 
 
-int dp_turn_on_offload_pkt_capture_on_single_iface(uint16_t port_id);
-int dp_turn_off_offload_pkt_capture_on_single_iface(uint16_t port_id);
+int dp_enable_port_offload_pkt_capture(uint16_t port_id);
+int dp_disable_port_offload_pkt_capture(uint16_t port_id);
 
-int dp_turn_on_offload_pkt_capture_on_all_ifaces(void);
-int dp_turn_off_offload_pkt_capture_on_all_ifaces(void);
+int dp_disable_pkt_capture_on_all_ifaces(void);
 
 int dp_destroy_default_flow(struct dp_port *port);
 
-void dp_configure_packet_capture_action(uint8_t *encaped_mirror_hdr,
+void dp_configure_pkt_capture_action(uint8_t *encaped_mirror_hdr,
 										struct rte_flow_action_raw_encap *encap_action,
 										struct rte_flow_action_port_id *port_id_action,
 										struct rte_flow_action *sub_action);

proto/dpdk.proto

@@ -532,7 +532,7 @@ message DeleteFirewallRuleResponse {
 
 message CapturedInterface {
 	CaptureInterfaceType interface_type = 1;
-	oneof interface_info {
+	oneof spec {
 		bytes vf_name = 2;
 		uint32 pf_index = 3;
 	}
@@ -554,7 +554,7 @@ message CaptureStopRequest {
 
 message CaptureStopResponse {
 	Status status = 1;
-	uint32 captured_interface_cnt = 2;
+	uint32 stopped_interface_cnt = 2;
 }
 
 service DPDKonmetal {

src/dp_port.c

@@ -459,7 +459,6 @@ static int dp_install_vf_init_rte_rules(uint32_t port_id)
 {
 	int ret;
 
-	// too long, thus using a ret variable
 	ret = dp_install_jump_rule_in_default_group(port_id, DP_RTE_FLOW_VNET_GROUP);
 	if (DP_FAILED(ret)) {
 		DPS_LOG_ERR("Cannot install default jump rule", DP_LOG_PORTID(port_id), DP_LOG_RET(ret));
@@ -532,7 +531,6 @@ int dp_port_stop(uint16_t port_id)
 	if (!port)
 		return DP_ERROR;
 
-	// not really resolve the issue, but let's do it explicitly
 	if (DP_FAILED(dp_destroy_default_flow(port)))
 		return DP_ERROR;
 

src/grpc/dp_async_grpc.cpp

@@ -543,19 +543,19 @@ void CreateNatCall::ParseReply(struct dpgrpc_reply* reply)
 
 const char* CaptureStopCall::FillRequest(__rte_unused struct dpgrpc_request* request)
 {
-	DPGRPC_LOG_INFO("Stop packet capture");
+	DPGRPC_LOG_INFO("Stopping packet capture");
 
 	return NULL;
 }
 void CaptureStopCall::ParseReply(struct dpgrpc_reply* reply)
 {
-	reply_.set_captured_interface_cnt((uint32_t)reply->capture_stop.port_cnt);
+	reply_.set_stopped_interface_cnt((uint32_t)reply->capture_stop.port_cnt);
 }
 
 const char* CaptureStartCall::FillRequest(struct dpgrpc_request* request)
 {
 
-	DPGRPC_LOG_INFO("Start packet capture",
+	DPGRPC_LOG_INFO("Starting packet capture",
 				DP_LOG_IPV6STR(request_.sink_node_ip().address().c_str()),
 				DP_LOG_PORT(request_.udp_src_port()),
 				DP_LOG_PORT(request_.udp_dst_port()));
@@ -573,34 +573,32 @@ const char* CaptureStartCall::FillRequest(struct dpgrpc_request* request)
 	if (request_.interfaces_size() > DP_CAPTURE_MAX_PORT_NUM)
 		return "Too many interfaces to be captured";
 
-	request->start_capture.filled_interface_info_count = 0;
+	request->start_capture.interface_count = 0;
 	for (int i = 0; i < request_.interfaces_size(); ++i) {
 		if (!GrpcConv::GrpcToDpCaptureInterfaceType(request_.interfaces(i).interface_type(), &request->start_capture.interfaces[i].type)) {
-			return "Invalid interface_type";
+			return "Invalid interfaces.interface_type";
 		}
 
 		switch (request->start_capture.interfaces[i].type) {
 		case DP_CAPTURE_IFACE_TYPE_SINGLE_VF:
 			DPGRPC_LOG_INFO("Set packet capture interface vf",
 							DP_LOG_PORT_TYPE(request_.interfaces(i).interface_type()),
 							DP_LOG_IFACE(request_.interfaces(i).vf_name().c_str()));
-			if (SNPRINTF_FAILED(request->start_capture.interfaces[i].interface_info.iface_id, request_.interfaces(i).vf_name()))
+			if (SNPRINTF_FAILED(request->start_capture.interfaces[i].spec.iface_id, request_.interfaces(i).vf_name()))
 				return "Invalid interface_id";
 			break;
 		case DP_CAPTURE_IFACE_TYPE_SINGLE_PF:
 			DPGRPC_LOG_INFO("Set packet capture interface pf",
 							DP_LOG_PORT_TYPE(request_.interfaces(i).interface_type()),
 							DP_LOG_IFACE_INDEX(request_.interfaces(i).pf_index()));
-			// TODO: maybe a validity check here for indexes
-			request->start_capture.interfaces[i].interface_info.pf_index = request_.interfaces(i).pf_index();
+			request->start_capture.interfaces[i].spec.pf_index = request_.interfaces(i).pf_index();
 			break;
 		}
 
-		request->start_capture.filled_interface_info_count++;
+		request->start_capture.interface_count++;
 	}
 	return NULL;
 }
-
 void CaptureStartCall::ParseReply(__rte_unused struct dpgrpc_reply* reply)
 {
 }
@@ -613,7 +611,6 @@ const char* GetNatCall::FillRequest(struct dpgrpc_request* request)
 		return "Invalid interface_id";
 	return NULL;
 }
-
 void GetNatCall::ParseReply(struct dpgrpc_reply* reply)
 {
 	IpAddress *nat_ip;

src/grpc/dp_grpc_conv.cpp

@@ -108,7 +108,6 @@ bool GrpcToDpFwallPort(int32_t grpc_port, uint32_t *dp_port)
 		return false;
 	*dp_port = port;
 	return true;
-
 }
 
 bool GrpcToDpCaptureInterfaceType(const CaptureInterfaceType& grpc_type, enum dpgrpc_capture_iface_type *dp_capture_iface_type)

src/grpc/dp_grpc_impl.c

@@ -886,41 +886,43 @@ static int dp_process_get_version(struct dp_grpc_responder *responder)
 
 static int dp_process_capture_start(struct dp_grpc_responder *responder)
 {
-	struct dpgrpc_capture_config *request = &responder->request.start_capture;
-	struct dpgrpc_capture_stat	*reply = dp_grpc_single_reply(responder);
-	int port_id = -1, status = DP_GRPC_OK;
+	struct dpgrpc_capture *request = &responder->request.start_capture;
+	int port_id = -1;
+	int status = DP_GRPC_OK;
 
-	dp_set_capture_node_ipv6_addr(request->dst_addr6);
-	dp_set_capture_udp_src_port(request->udp_src_port);
-	dp_set_capture_udp_dst_port(request->udp_dst_port);
+	if (dp_get_capture_enabled())
+		return DP_GRPC_ERR_CAPTURE_ALREADY_SET;
 
+	dp_set_capture_hdr_config(request->dst_addr6, request->udp_src_port, request->udp_dst_port);
 	dp_set_capture_enabled(true);
 
-	for (int i = 0; i < request->filled_interface_info_count; ++i) {
+	for (int i = 0; i < request->interface_count; ++i) {
 		switch (request->interfaces[i].type) {
 		case DP_CAPTURE_IFACE_TYPE_SINGLE_VF:
-			port_id = dp_get_portid_with_vm_handle(request->interfaces[i].interface_info.iface_id);
+			port_id = dp_get_portid_with_vm_handle(request->interfaces[i].spec.iface_id);
 			break;
 		case DP_CAPTURE_IFACE_TYPE_SINGLE_PF:
-			//index check is done on the grpc client side
-			port_id = request->interfaces[i].interface_info.pf_index == 0 ? dp_port_get_pf0_id() : dp_port_get_pf1_id();
+			if (request->interfaces[i].spec.pf_index >= DP_MAX_PF_PORTS)
+				return DP_GRPC_ERR_CAPTURE_INVALID_PORT_ID;
+			port_id = request->interfaces[i].spec.pf_index == 0 ? dp_port_get_pf0_id() : dp_port_get_pf1_id();
 			break;
 		}
 
 		if (DP_FAILED(port_id)) {
-			reply->interface = request->interfaces[i];
-			status = DP_GRPC_ERR_CAPTURE_INIT_INVALID_PORT_ID;
+			DPS_LOG_WARNING("Got invalid port id when initializing capturing", DP_LOG_PORTID(port_id));
+			status = DP_GRPC_ERR_CAPTURE_INVALID_PORT_ID;
 			break;
 		}
 
-		status = dp_turn_on_offload_pkt_capture_on_single_iface(port_id);
-		if (DP_FAILED(status))
-			break;	// stop continuing to turn on offload capture on other interfaces
+		status = dp_enable_port_offload_pkt_capture(port_id);
+		if (DP_FAILED(status)) // stop continuing to turn on offload capture on other interfaces, if capturing init failed on any port. abort and rollback.
+			break;
 	}
 
+	// try to turn off capture on all interfaces if any of them failed to turn on
 	if (DP_FAILED(status)) {
-		if (DP_FAILED(dp_turn_off_offload_pkt_capture_on_all_ifaces()))	// try to turn off capture on all interfaces
-			status = DP_GRPC_ERR_CAPTURE_INIT_CANNOT_ROLLBACK;
+		if (DP_FAILED(dp_disable_pkt_capture_on_all_ifaces()))
+			status = DP_GRPC_ERR_CAPTURE_CANNOT_ROLLBACK;
 	}
 
 	return status;
@@ -929,10 +931,13 @@ static int dp_process_capture_start(struct dp_grpc_responder *responder)
 static int dp_process_capture_stop(struct dp_grpc_responder *responder)
 {
 	struct dpgrpc_capture_stop	*reply = dp_grpc_single_reply(responder);
-	int ret = dp_turn_off_offload_pkt_capture_on_all_ifaces();
+	int ret;
 
-	if (DP_FAILED(ret))
-		return DP_GRPC_ERR_CAPTURE_CANNOT_STOP;
+	ret = dp_disable_pkt_capture_on_all_ifaces();
+	if (DP_FAILED(ret)) {
+		DPS_LOG_ERR("Failed to stop packet capture on all interfaces"); // it is problematic that we cannot rollback here
+		return ret;
+	}
 
 	reply->port_cnt = ret;
 	return DP_GRPC_OK;
@@ -1072,7 +1077,7 @@ void dp_process_request(struct rte_mbuf *m)
 	}
 
 	if (DP_FAILED(ret)) {
-		// as gRPC errors are explicitely defined due to API reasons
+		// as gRPC errors are explicitly defined due to API reasons
 		// extract the proper value from the standard (negative) retvals
 		ret = dp_errcode_to_grpc_errcode(ret);
 		DPGRPC_LOG_WARNING("Failed request", DP_LOG_GRPCREQUEST(responder.request.type), DP_LOG_GRPCRET(ret));

src/monitoring/dp_graphtrace.c

@@ -20,7 +20,6 @@ int _dp_graphtrace_flags;
 bool _dp_graphtrace_enabled = false;
 
 static struct dp_graphtrace graphtrace;
-static bool offload_enabled;
 
 static int dp_graphtrace_init_memzone(void)
 {
@@ -44,8 +43,6 @@ static int dp_graphtrace_init_memzone(void)
 		return DP_ERROR;
 	}
 
-	offload_enabled = dp_conf_is_offload_enabled();
-
 	return DP_OK;
 }