From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <ferruh.yigit@intel.com>
Received: from mga07.intel.com (mga07.intel.com [134.134.136.100])
 by dpdk.org (Postfix) with ESMTP id 4C1342A6C
 for <dev@dpdk.org>; Fri,  6 Jan 2017 18:11:22 +0100 (CET)
Received: from orsmga004.jf.intel.com ([10.7.209.38])
 by orsmga105.jf.intel.com with ESMTP; 06 Jan 2017 09:11:20 -0800
X-ExtLoop1: 1
X-IronPort-AV: E=Sophos;i="5.33,325,1477983600"; d="scan'208";a="46349859"
Received: from fyigit-mobl1.ger.corp.intel.com (HELO [10.237.220.38])
 ([10.237.220.38])
 by orsmga004.jf.intel.com with ESMTP; 06 Jan 2017 09:11:20 -0800
To: Wei Zhao <wei.zhao1@intel.com>, dev@dpdk.org
References: <1483084390-53159-1-git-send-email-wei.zhao1@intel.com>
 <1483084390-53159-13-git-send-email-wei.zhao1@intel.com>
Cc: Wenzhuo Lu <wenzhuo.lu@intel.com>
From: Ferruh Yigit <ferruh.yigit@intel.com>
Message-ID: <155c938b-0d58-0eba-bc0f-7851429f0404@intel.com>
Date: Fri, 6 Jan 2017 17:11:19 +0000
User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:45.0) Gecko/20100101
 Thunderbird/45.6.0
MIME-Version: 1.0
In-Reply-To: <1483084390-53159-13-git-send-email-wei.zhao1@intel.com>
Content-Type: text/plain; charset=windows-1252
Content-Transfer-Encoding: 8bit
Subject: Re: [dpdk-dev] [PATCH v2 12/18] net/ixgbe: parse ethertype filter
X-BeenThere: dev@dpdk.org
X-Mailman-Version: 2.1.15
Precedence: list
List-Id: DPDK patches and discussions <dev.dpdk.org>
List-Unsubscribe: <http://dpdk.org/ml/options/dev>,
 <mailto:dev-request@dpdk.org?subject=unsubscribe>
List-Archive: <http://dpdk.org/ml/archives/dev/>
List-Post: <mailto:dev@dpdk.org>
List-Help: <mailto:dev-request@dpdk.org?subject=help>
List-Subscribe: <http://dpdk.org/ml/listinfo/dev>,
 <mailto:dev-request@dpdk.org?subject=subscribe>
X-List-Received-Date: Fri, 06 Jan 2017 17:11:22 -0000

On 12/30/2016 7:53 AM, Wei Zhao wrote:
> check if the rule is a ethertype rule, and get the ethertype info.
> Signed-off-by: Wei Zhao <wei.zhao1@intel.com>
> Signed-off-by: Wenzhuo Lu <wenzhuo.lu@intel.com>
> 
> ---
> 
> v2:add new error set function
> ---

<...>

> +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?

> +		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) ?

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

Isn't this duplicate with above check?

> +		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?

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