Hi, Yigit

-----Original Message-----
From: Yigit, Ferruh
Sent: Saturday, January 7, 2017 1:11 AM
To: Zhao1, Wei <redacted>; dev@dpdk.org
Cc: Lu, Wenzhuo <redacted>
Subject: Re: [dpdk-dev] [PATCH v2 12/18] net/ixgbe: parse ethertype filter

On 12/30/2016 7:53 AM, Wei Zhao wrote:

quoted

check if the rule is a ethertype rule, and get the ethertype info.
Signed-off-by: Wei Zhao <redacted>
Signed-off-by: Wenzhuo Lu <redacted>

---

v2:add new error set function
---

<...>

quoted

+static int
+ixgbe_parse_ethertype_filter(const struct rte_flow_attr *attr,
+			     const struct rte_flow_item pattern[],
+			     const struct rte_flow_action actions[],
+			     struct rte_eth_ethertype_filter *filter,
+			     struct rte_flow_error *error) {
+	int ret;
+
+	ret = cons_parse_ethertype_filter(attr, pattern,
+					actions, filter, error);
+
+	if (ret)
+		return ret;
+
+	/* Ixgbe doesn't support MAC address. */
+	if (filter->flags & RTE_ETHTYPE_FLAGS_MAC) {
+		memset(filter, 0, sizeof(struct rte_eth_ethertype_filter));

Is memset required for error cases, if so is other error cases also require it?

Yes, Ok , I will do as your suggestion.

quoted

+		rte_flow_error_set(error, EINVAL,
+				RTE_FLOW_ERROR_TYPE_ITEM,
+				NULL, "Not supported by ethertype filter");
+		return -rte_errno;
+	}
+
+	if (filter->queue >= IXGBE_MAX_RX_QUEUE_NUM)
+		return -rte_errno;
+
+	if (filter->ether_type == ETHER_TYPE_IPv4 ||
+		filter->ether_type == ETHER_TYPE_IPv6) {
+		PMD_DRV_LOG(ERR, "unsupported ether_type(0x%04x) in"
+			" ethertype filter.", filter->ether_type);

Not sure about this log here, specially it is in ERR level.
This function is returning error, and public API will return an error, if we want
to notify user with a log, should app do this as library
(here) should do this? More comment welcome?

btw, should rte_flow_error_set() used here (and below) ?

Yes, I will change to rte_flow_error_set() 

quoted

+		return -rte_errno;
+	}
+
+	if (filter->flags & RTE_ETHTYPE_FLAGS_MAC) {

Isn't this duplicate with above check?

quoted

+		PMD_DRV_LOG(ERR, "mac compare is unsupported.");
+		return -rte_errno;
+	}
+
+	if (filter->flags & RTE_ETHTYPE_FLAGS_DROP) {

Just to double check, isn't drop action by ether filters?

Yse , ixgbe do not, but i40e is.

quoted

+		PMD_DRV_LOG(ERR, "drop option is unsupported.");
+		return -rte_errno;
+	}
+
+	return 0;
+}
+
+/**

<...>