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

@@ -38,6 +38,11 @@ const char *dp_strerror_verbose(int error);
 	ERR(ITERATOR,							207) \
 	ERR(OUT_OF_MEMORY,						208) \
 	ERR(LIMIT_REACHED,						209) \
+	ERR(ALREADY_ACTIVE,						210) \
+	ERR(NOT_ACTIVE,							211) \
+	ERR(ROLLBACK,							212) \
+	ERR(RTE_RULE_ADD,						213) \
+	ERR(RTE_RULE_DEL,						214) \
 	/* Specific errors */ \
 	ERR(ROUTE_EXISTS,						301) \
 	ERR(ROUTE_NOT_FOUND,					302) \
@@ -61,16 +66,7 @@ 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) \
+
 
 #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,19 +31,21 @@ 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);
+const struct dp_capture_hdr_config *dp_get_capture_hdr_config(void);
 
 void dp_set_capture_enabled(bool enabled);
 
-bool dp_get_capture_enabled(void);
+bool dp_is_capture_enabled(void);
 
 #ifdef __cplusplus
 }

include/rte_flow/dp_rte_flow_capture.h

@@ -0,0 +1,32 @@
+#ifndef __INCLUDE_DP_RTE_FLOW_CAPTURE_H__
+#define __INCLUDE_DP_RTE_FLOW_CAPTURE_H__
+
+#ifdef __cplusplus
+extern "C" {
+#endif
+
+#include <stdint.h>
+#include <stdbool.h>
+#include <rte_flow.h>
+#include "dp_port.h"
+
+
+int dp_install_jump_rule_in_default_group(uint16_t port_id, uint32_t dst_group);
+
+int dp_enable_port_offload_pkt_capture(uint16_t port_id);
+int dp_disable_port_offload_pkt_capture(uint16_t port_id);
+
+int dp_disable_pkt_capture_on_all_ifaces(void);
+
+int dp_destroy_default_flow(struct dp_port *port);
+
+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);
+
+#ifdef __cplusplus
+}
+#endif
+
+#endif

include/rte_flow/dp_rte_flow_init.h

@@ -12,23 +12,6 @@ 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_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_turn_on_offload_pkt_capture_on_all_ifaces(void);
-int dp_turn_off_offload_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,
-										struct rte_flow_action_raw_encap *encap_action,
-										struct rte_flow_action_port_id *port_id_action,
-										struct rte_flow_action *sub_action);
-
 #ifdef ENABLE_VIRTSVC
 int dp_install_isolated_mode_virtsvc(int port_id, uint8_t proto_id, const uint8_t svc_ipv6[16], uint16_t svc_port);
 #endif

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

@@ -13,6 +13,7 @@
 #include "nodes/rx_node.h"
 #include "rte_flow/dp_rte_flow_init.h"
 #include "rte_flow/dp_rte_flow.h"
+#include "rte_flow/dp_rte_flow_capture.h"
 #include "monitoring/dp_graphtrace.h"
 
 static const struct rte_eth_conf port_conf_default = {
@@ -459,7 +460,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 +532,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)