[dpdk-dev] [PATCH v4] ethdev: add sanity checks in control APIs

Min Hu (Connor) humin29 at huawei.com
Fri Apr 16 09:00:39 CEST 2021


Thanks Kevin,
	all is fixed in v6, please review it, thanks.
	Some comments are below.

在 2021/4/15 20:04, Kevin Traynor 写道:
> On 15/04/2021 01:52, Min Hu (Connor) wrote:
>> This patch adds more sanity checks in control path APIs.
>>
> 
> Hi Connor,
> 
> A few general comments,
> 
> --
> Some of the functions have unit tests, you could consider adding unit
> tests for the new checks. Considering the checks are not subtle and
> unlikely to be messed up in future, not adding unit tests is not a
> blocker imho.
> 
> --
> It took me a while to get what you meant with "by NULL". It's usage
> seems like in "Death by taxes". Perhaps "because NULL ptr" would be a
> better way to phrase this generically, but I think it is more useful to
> say what is NULL.
> 
> e.g. "Failed to convert NULL to string\n" is very generic and would be
> better as "Failed to convert NULL link to string\n" . ok, still a bit
> generic but more of a clue.
> 
> I won't comment on each log message individually but I've added a few
> suggestions here and there.
> --
> 
> Did you check the usage of these functions in DPDK, and if the return
> value is handled ok? e.g. RTE_ETH_FOREACH_MATCHING_DEV will keep calling
> iterator functions. I'm not sure that having a return check is needed in
> that case, but there could be other cases where you want to take some
> different action now.
> 
As iterator functions are all APIs, they may be used by APP directly.
I think param check is necessary.
> some other comments inlined,
> 
>> Fixes: 214ed1acd125 ("ethdev: add iterator to match devargs input")
>> Fixes: 3d98f921fbe9 ("ethdev: unify prefix for static functions and variables")
>> Fixes: 0366137722a0 ("ethdev: check for invalid device name")
>> Fixes: d948f596fee2 ("ethdev: fix port data mismatched in multiple process model")
>> Fixes: 5b7ba31148a8 ("ethdev: add port ownership")
>> Fixes: f8244c6399d9 ("ethdev: increase port id range")
>> Cc: stable at dpdk.org
>>
>> Signed-off-by: Min Hu (Connor) <humin29 at huawei.com>
>> ---
>> v4:
>> * add error logging.
>> * delete check in control path API.
>>
>> v3:
>> * set port_id checked first.
>> * add error logging.
>>
>> v2:
>> * Removed unnecessary checks.
>> * Deleted checks in internal API.
>> * Added documentation in the header file.
>> ---
>>   lib/librte_ethdev/rte_ethdev.c | 288 ++++++++++++++++++++++++++++++++++++++---
>>   lib/librte_ethdev/rte_ethdev.h |  20 ++-
>>   2 files changed, 287 insertions(+), 21 deletions(-)
>>
>> diff --git a/lib/librte_ethdev/rte_ethdev.c b/lib/librte_ethdev/rte_ethdev.c
>> index 6b5cfd6..e734a30 100644
>> --- a/lib/librte_ethdev/rte_ethdev.c
>> +++ b/lib/librte_ethdev/rte_ethdev.c
>> @@ -199,6 +199,16 @@ rte_eth_iterator_init(struct rte_dev_iterator *iter, const char *devargs_str)
>>   	char *cls_str = NULL;
>>   	int str_size;
>>   
>> +	if (iter == NULL) {
>> +		RTE_ETHDEV_LOG(ERR, "Failed to iterator init for NULL\n");
> 
> "Failed to init iterator for NULL iterator\n"
> 
>> +		return -EINVAL;
>> +	}
>> +
>> +	if (devargs_str == NULL) {
>> +		RTE_ETHDEV_LOG(ERR, "Failed to iterate matching NULL\n");
> 
> "Failed to init iterator for NULL devargs\n"
> 
>> +		return -EINVAL;
>> +	}
>> +
>>   	memset(iter, 0, sizeof(*iter));
>>   
>>   	/*
>> @@ -293,6 +303,11 @@ rte_eth_iterator_init(struct rte_dev_iterator *iter, const char *devargs_str)
>>   uint16_t
>>   rte_eth_iterator_next(struct rte_dev_iterator *iter)
>>   {
>> +	if (iter == NULL) {
>> +		RTE_ETHDEV_LOG(ERR, "Failed to iterate next for NULL\n");
>> +		return RTE_MAX_ETHPORTS;
>> +	}
>> +
>>   	if (iter->cls == NULL) /* invalid ethdev iterator */
>>   		return RTE_MAX_ETHPORTS;
>>   
>> @@ -322,6 +337,11 @@ rte_eth_iterator_next(struct rte_dev_iterator *iter)
>>   void
>>   rte_eth_iterator_cleanup(struct rte_dev_iterator *iter)
>>   {
>> +	if (iter == NULL) {
>> +		RTE_ETHDEV_LOG(ERR, "Failed to iterator clear up for NULL\n");
>> +		return;
>> +	}
>> +
>>   	if (iter->bus_str == NULL)
>>   		return; /* nothing to free in pure class filter */
>>   	free(RTE_CAST_FIELD(iter, bus_str, char *)); /* workaround const */
>> @@ -622,6 +642,11 @@ rte_eth_find_next_owned_by(uint16_t port_id, const uint64_t owner_id)
>>   int
>>   rte_eth_dev_owner_new(uint64_t *owner_id)
>>   {
>> +	if (owner_id == NULL) {
>> +		RTE_ETHDEV_LOG(ERR, "Failed to get owner id by NULL\n");
>> +		return -EINVAL;
>> +	}
>> +
>>   	eth_dev_shared_data_prepare();
>>   
>>   	rte_spinlock_lock(&eth_dev_shared_data->ownership_lock);
>> @@ -645,6 +670,12 @@ eth_dev_owner_set(const uint16_t port_id, const uint64_t old_owner_id,
>>   		return -ENODEV;
>>   	}
>>   
>> +	if (new_owner == NULL) {
>> +		RTE_ETHDEV_LOG(ERR, "Failed to set ethdev port %u owner to NULL\n",
>> +			port_id);
>> +		return -EINVAL;
>> +	}
>> +
>>   	if (!eth_is_valid_owner_id(new_owner->id) &&
>>   	    !eth_is_valid_owner_id(old_owner_id)) {
>>   		RTE_ETHDEV_LOG(ERR,
>> @@ -738,23 +769,30 @@ rte_eth_dev_owner_delete(const uint64_t owner_id)
>>   int
>>   rte_eth_dev_owner_get(const uint16_t port_id, struct rte_eth_dev_owner *owner)
>>   {
>> -	int ret = 0;
>> -	struct rte_eth_dev *ethdev = &rte_eth_devices[port_id];
>> -
>> -	eth_dev_shared_data_prepare();
>> +	struct rte_eth_dev *ethdev;
>>   
>> -	rte_spinlock_lock(&eth_dev_shared_data->ownership_lock);
>> +	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -1);
>>   
>> -	if (port_id >= RTE_MAX_ETHPORTS || !eth_dev_is_allocated(ethdev)) {
>> +	ethdev = &rte_eth_devices[port_id];
>> +	if (!eth_dev_is_allocated(ethdev)) {
>>   		RTE_ETHDEV_LOG(ERR, "Port id %"PRIu16" is not allocated\n",
>>   			port_id);
>> -		ret = -ENODEV;
>> -	} else {
>> -		rte_memcpy(owner, &ethdev->data->owner, sizeof(*owner));
>> +		return -ENODEV;
>>   	}
>>   
>> +	if (owner == NULL) {
>> +		RTE_ETHDEV_LOG(ERR, "Failed to get ethdev port %u owner by NULL\n",
>> +			port_id);
>> +		return -EINVAL;
>> +	}
> 
> This fn uses both %u and %"PRIu16" for port_id
> 
>> +
>> +	eth_dev_shared_data_prepare();
>> +
>> +	rte_spinlock_lock(&eth_dev_shared_data->ownership_lock);
>> +	rte_memcpy(owner, &ethdev->data->owner, sizeof(*owner));
>>   	rte_spinlock_unlock(&eth_dev_shared_data->ownership_lock);
>> -	return ret;
>> +
>> +	return 0;
>>   }
>>   
>>   int
>> @@ -820,7 +858,7 @@ rte_eth_dev_get_port_by_name(const char *name, uint16_t *port_id)
>>   {
>>   	uint16_t pid;
>>   
>> -	if (name == NULL) {
>> +	if (name == NULL || port_id == NULL) {
> 
> Most of the other new checks have individual checks for NULL ptrs, this
> one doesn't. I'm not sure if it needs individual checks, but it is
> better to be consistent.
> 
>>   		RTE_ETHDEV_LOG(ERR, "Null pointer is specified\n");
>>   		return -EINVAL;
>>   	}
>> @@ -1299,6 +1337,12 @@ rte_eth_dev_configure(uint16_t port_id, uint16_t nb_rx_q, uint16_t nb_tx_q,
>>   
>>   	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
>>   
>> +	if (dev_conf == NULL) {
>> +		RTE_ETHDEV_LOG(ERR, "Failed to configure ethdev port %u to NULL\n",
>> +			port_id);
>> +		return -EINVAL;
>> +	}
>> +
>>   	dev = &rte_eth_devices[port_id];
>>   
>>   	RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->dev_configure, -ENOTSUP);
>> @@ -2139,6 +2183,12 @@ rte_eth_rx_hairpin_queue_setup(uint16_t port_id, uint16_t rx_queue_id,
>>   
>>   	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
>>   
>> +	if (conf == NULL) {
>> +		RTE_ETHDEV_LOG(ERR, "Failed to setup ethdev port %u Rx hairpin queue to NULL\n",
>> +			port_id);
>> +		return -EINVAL;
>> +	}
>> +
>>   	dev = &rte_eth_devices[port_id];
>>   	if (rx_queue_id >= dev->data->nb_rx_queues) {
>>   		RTE_ETHDEV_LOG(ERR, "Invalid RX queue_id=%u\n", rx_queue_id);
>> @@ -2310,6 +2360,13 @@ rte_eth_tx_hairpin_queue_setup(uint16_t port_id, uint16_t tx_queue_id,
>>   	int ret;
>>   
>>   	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
>> +
>> +	if (conf == NULL) {
>> +		RTE_ETHDEV_LOG(ERR, "Failed to setup ethdev port %u Tx hairpin queue to NULL\n",
>> +			port_id);
>> +		return -EINVAL;
>> +	}
>> +
>>   	dev = &rte_eth_devices[port_id];
>>   	if (tx_queue_id >= dev->data->nb_tx_queues) {
>>   		RTE_ETHDEV_LOG(ERR, "Invalid TX queue_id=%u\n", tx_queue_id);
>> @@ -2459,6 +2516,11 @@ int
>>   rte_eth_tx_buffer_set_err_callback(struct rte_eth_dev_tx_buffer *buffer,
>>   		buffer_tx_error_fn cbfn, void *userdata)
>>   {
>> +	if (buffer == NULL) {
>> +		RTE_ETHDEV_LOG(ERR, "Failed to set error callback for NULL\n");
>> +		return -EINVAL;
>> +	}
>> +
>>   	buffer->error_callback = cbfn;
>>   	buffer->error_userdata = userdata;
>>   	return 0;
>> @@ -2607,6 +2669,13 @@ rte_eth_link_get(uint16_t port_id, struct rte_eth_link *eth_link)
>>   	struct rte_eth_dev *dev;
>>   
>>   	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
>> +
>> +	if (eth_link == NULL) {
>> +		RTE_ETHDEV_LOG(ERR, "Failed to get ethdev port %u link by NULL\n",
> 
> "by NULL" does not read nicely to me. How about
> "Failed to get ethdev port %u for NULL link"
> 
>> +			port_id);
>> +		return -EINVAL;
>> +	}
>> +
>>   	dev = &rte_eth_devices[port_id];
>>   
>>   	if (dev->data->dev_conf.intr_conf.lsc &&
>> @@ -2627,6 +2696,13 @@ rte_eth_link_get_nowait(uint16_t port_id, struct rte_eth_link *eth_link)
>>   	struct rte_eth_dev *dev;
>>   
>>   	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
>> +
>> +	if (eth_link == NULL) {
>> +		RTE_ETHDEV_LOG(ERR, "Failed to get ethdev port %u nowait link by NULL\n",
>> +			port_id);
> 
> Same comment as for previous function, except you'll probably want to
> keep nowait somewhere to distinguish.
> "Failed to get nowait ethdev port %u for NULL link" ?
> 
>> +		return -EINVAL;
>> +	}
>> +
>>   	dev = &rte_eth_devices[port_id];
>>   
>>   	if (dev->data->dev_conf.intr_conf.lsc &&
>> @@ -2667,6 +2743,16 @@ rte_eth_link_speed_to_str(uint32_t link_speed)
>>   int
>>   rte_eth_link_to_str(char *str, size_t len, const struct rte_eth_link *eth_link)
>>   {
> 
> The messages are very generic, especially the second one. Suggestions below
> 
>> +	if (str == NULL) {
>> +		RTE_ETHDEV_LOG(ERR, "Failed to convert link to NULL\n");
> 
> "Failed to convert link to NULL string\n"
> 
>> +		return -EINVAL;
>> +	}
> 
> You should probably also check the 'len' param for str here also.
> 
>> +
>> +	if (eth_link == NULL) {
>> +		RTE_ETHDEV_LOG(ERR, "Failed to convert NULL to string\n");
> 
> "Failed to convert NULL link to string\n"
> 
>> +		return -EINVAL;
>> +	}
>> +
>>   	if (eth_link->link_status == ETH_LINK_DOWN)
>>   		return snprintf(str, len, "Link down");
>>   	else
>> @@ -2685,6 +2771,12 @@ rte_eth_stats_get(uint16_t port_id, struct rte_eth_stats *stats)
>>   
>>   	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
>>   
>> +	if (stats == NULL) {
>> +		RTE_ETHDEV_LOG(ERR, "Failed to get ethdev port %u stats by NULL\n",
>> +			port_id);
>> +		return -EINVAL;
>> +	}
>> +
>>   	dev = &rte_eth_devices[port_id];
>>   	memset(stats, 0, sizeof(*stats));
>>   
>> @@ -3258,6 +3350,13 @@ rte_eth_dev_fw_version_get(uint16_t port_id, char *fw_version, size_t fw_size)
>>   	struct rte_eth_dev *dev;
>>   
>>   	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
>> +
>> +	if (fw_version == NULL) {
>> +		RTE_ETHDEV_LOG(ERR, "Failed to get ethdev port %u fw version by NULL\n",
>> +			port_id);
>> +		return -EINVAL;
>> +	}
> 
> Some of the drivers already check for NULL ptrs and some don't. It means
> some checks could be removed from the drivers, but it doesn't seem a
> worthwhile effort unless they are needing some rework anyway.
Agreed, this may be fixed in next patch.
> 
> 'fw_size == 0' could also be checked here
> 
>> +
>>   	dev = &rte_eth_devices[port_id];
>>   
>>   	RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->fw_version_get, -ENOTSUP);
>> @@ -3278,6 +3377,14 @@ rte_eth_dev_info_get(uint16_t port_id, struct rte_eth_dev_info *dev_info)
>>   	};
>>   	int diag;
>>   
>> +	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
>> +
>> +	if (dev_info == NULL) {
>> +		RTE_ETHDEV_LOG(ERR, "Failed to get ethdev port %u info by NULL\n",
>> +			port_id);
>> +		return -EINVAL;
>> +	}
>> +
>>   	/*
>>   	 * Init dev_info before port_id check since caller does not have
>>   	 * return status and does not know if get is successful or not.
>> @@ -3285,7 +3392,6 @@ rte_eth_dev_info_get(uint16_t port_id, struct rte_eth_dev_info *dev_info)
>>   	memset(dev_info, 0, sizeof(struct rte_eth_dev_info));
>>   	dev_info->switch_info.domain_id = RTE_ETH_DEV_SWITCH_DOMAIN_ID_INVALID;
>>   
>> -	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
> 
> This is a slight change in behaviour. With an invalid port, previously
> dev_info was memset to 0. Does it need to be? I guess no.
> 
> At the very least the comment block is incorrect and should be changed.
> 
>>   	dev = &rte_eth_devices[port_id];
>>   
>>   	dev_info->rx_desc_lim = lim;
>> @@ -3326,6 +3432,13 @@ rte_eth_dev_get_supported_ptypes(uint16_t port_id, uint32_t ptype_mask,
>>   	const uint32_t *all_ptypes;
>>   
>>   	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
>> +
>> +	if (ptypes == NULL) {
>> +		RTE_ETHDEV_LOG(ERR, "Failed to get ethdev port %u supported ptypes by NULL\n",
>> +			port_id);
>> +		return -EINVAL;
>> +	}
>> +
> 
> 'num == 0' could be checked here too.
> 
>>   	dev = &rte_eth_devices[port_id];
>>   	RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->dev_supported_ptypes_get, 0);
>>   	all_ptypes = (*dev->dev_ops->dev_supported_ptypes_get)(dev);
>> @@ -3435,6 +3548,13 @@ rte_eth_macaddr_get(uint16_t port_id, struct rte_ether_addr *mac_addr)
>>   	struct rte_eth_dev *dev;
>>   
>>   	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
>> +
>> +	if (mac_addr == NULL) {
>> +		RTE_ETHDEV_LOG(ERR, "Failed to get ethdev port %u MAC address by NULL\n",
> 
> "by NULL" doesn't read correctly.
> "Failed to get ethdev port %u MAC address for NULL param\n"
> 
>> +			port_id);
>> +		return -EINVAL;
>> +	}
>> +
>>   	dev = &rte_eth_devices[port_id];
>>   	rte_ether_addr_copy(&dev->data->mac_addrs[0], mac_addr);
>>   
>> @@ -3448,6 +3568,12 @@ rte_eth_dev_get_mtu(uint16_t port_id, uint16_t *mtu)
>>   
>>   	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
>>   
>> +	if (mtu == NULL) {
>> +		RTE_ETHDEV_LOG(ERR, "Failed to get ethdev port %u MTU by NULL\n",
>> +			port_id);
>> +		return -EINVAL;
>> +	}
>> +
>>   	dev = &rte_eth_devices[port_id];
>>   	*mtu = dev->data->mtu;
>>   	return 0;
>> @@ -3696,6 +3822,13 @@ rte_eth_dev_flow_ctrl_get(uint16_t port_id, struct rte_eth_fc_conf *fc_conf)
>>   	struct rte_eth_dev *dev;
>>   
>>   	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
>> +
>> +	if (fc_conf == NULL) {
>> +		RTE_ETHDEV_LOG(ERR, "Failed to get ethdev port %u flow conf by NULL\n",
> 
> I would at least add "flow *ctrl* conf" e.g.
> "Failed to get ethdev port %u flow ctrl conf for NULL param\n"
> 
>> +			port_id);
>> +		return -EINVAL;
>> +	}
>> +
>>   	dev = &rte_eth_devices[port_id];
>>   	RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->flow_ctrl_get, -ENOTSUP);
>>   	memset(fc_conf, 0, sizeof(*fc_conf));
>> @@ -3708,6 +3841,13 @@ rte_eth_dev_flow_ctrl_set(uint16_t port_id, struct rte_eth_fc_conf *fc_conf)
>>   	struct rte_eth_dev *dev;
>>   
>>   	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
>> +
>> +	if (fc_conf == NULL) {
>> +		RTE_ETHDEV_LOG(ERR, "Failed to set ethdev port %u flow conf to NULL\n",
>> +			port_id);
>> +		return -EINVAL;
>> +	}
>> +
>>   	if ((fc_conf->send_xon != 0) && (fc_conf->send_xon != 1)) {
>>   		RTE_ETHDEV_LOG(ERR, "Invalid send_xon, only 0/1 allowed\n");
>>   		return -EINVAL;
>> @@ -3725,6 +3865,13 @@ rte_eth_dev_priority_flow_ctrl_set(uint16_t port_id,
>>   	struct rte_eth_dev *dev;
>>   
>>   	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
>> +
>> +	if (pfc_conf == NULL) {
>> +		RTE_ETHDEV_LOG(ERR, "Failed to set ethdev port %u priority flow conf to NULL\n",
>> +			port_id);
>> +		return -EINVAL;
>> +	}
>> +
>>   	if (pfc_conf->priority > (ETH_DCB_NUM_USER_PRIORITIES - 1)) {
>>   		RTE_ETHDEV_LOG(ERR, "Invalid priority, only 0-7 allowed\n");
>>   		return -EINVAL;
>> @@ -3744,9 +3891,6 @@ eth_check_reta_mask(struct rte_eth_rss_reta_entry64 *reta_conf,
>>   {
>>   	uint16_t i, num;
>>   
>> -	if (!reta_conf)
>> -		return -EINVAL;
>> -
>>   	num = (reta_size + RTE_RETA_GROUP_SIZE - 1) / RTE_RETA_GROUP_SIZE;
>>   	for (i = 0; i < num; i++) {
>>   		if (reta_conf[i].mask)
>> @@ -3763,9 +3907,6 @@ eth_check_reta_entry(struct rte_eth_rss_reta_entry64 *reta_conf,
>>   {
>>   	uint16_t i, idx, shift;
>>   
>> -	if (!reta_conf)
>> -		return -EINVAL;
>> -
>>   	if (max_rxq == 0) {
>>   		RTE_ETHDEV_LOG(ERR, "No receive queue is available\n");
>>   		return -EINVAL;
>> @@ -3796,6 +3937,13 @@ rte_eth_dev_rss_reta_update(uint16_t port_id,
>>   	int ret;
>>   
>>   	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
>> +
>> +	if (reta_conf == NULL) {
>> +		RTE_ETHDEV_LOG(ERR, "Failed to update ethdev port %u rss reta to NULL\n",
>> +			port_id);
>> +		return -EINVAL;
>> +	}
>> +
> 
> You may want to check reta_size as well in these rss functions?
> 
>>   	/* Check mask bits */
>>   	ret = eth_check_reta_mask(reta_conf, reta_size);
>>   	if (ret < 0)
>> @@ -3824,6 +3972,12 @@ rte_eth_dev_rss_reta_query(uint16_t port_id,
>>   
>>   	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
>>   
>> +	if (reta_conf == NULL) {
>> +		RTE_ETHDEV_LOG(ERR, "Failed to query ethdev port %u rss reta by NULL\n",
>> +			port_id);
>> +		return -EINVAL;
>> +	}
>> +
>>   	/* Check mask bits */
>>   	ret = eth_check_reta_mask(reta_conf, reta_size);
>>   	if (ret < 0)
>> @@ -3845,6 +3999,12 @@ rte_eth_dev_rss_hash_update(uint16_t port_id,
>>   
>>   	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
>>   
>> +	if (rss_conf == NULL) {
>> +		RTE_ETHDEV_LOG(ERR, "Failed to update ethdev port %u rss hash to NULL\n",
>> +			port_id);
>> +		return -EINVAL;
>> +	}
>> +
>>   	ret = rte_eth_dev_info_get(port_id, &dev_info);
>>   	if (ret != 0)
>>   		return ret;
>> @@ -3872,6 +4032,13 @@ rte_eth_dev_rss_hash_conf_get(uint16_t port_id,
>>   	struct rte_eth_dev *dev;
>>   
>>   	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
>> +
>> +	if (rss_conf == NULL) {
>> +		RTE_ETHDEV_LOG(ERR, "Failed to get ethdev port %u rss hash conf by NULL\n",
>> +			port_id);
>> +		return -EINVAL;
>> +	}
>> +
>>   	dev = &rte_eth_devices[port_id];
>>   	RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->rss_hash_conf_get, -ENOTSUP);
>>   	return eth_err(port_id, (*dev->dev_ops->rss_hash_conf_get)(dev,
>> @@ -4027,6 +4194,13 @@ rte_eth_dev_mac_addr_add(uint16_t port_id, struct rte_ether_addr *addr,
>>   	int ret;
>>   
>>   	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
>> +
>> +	if (addr == NULL) {
>> +		RTE_ETHDEV_LOG(ERR, "Failed to add ethdev port %u MAC address to NULL\n",
>> +			port_id);
> 
> This message is a bit confusing. Perhaps,
> "Failed to add NULL MAC address to ethdev port %u\n"
> 
> thanks,
> Kevin.
> 
>> +		return -EINVAL;
>> +	}
>> +
>>   	dev = &rte_eth_devices[port_id];
>>   	RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->mac_addr_add, -ENOTSUP);
>>   
>> @@ -4077,6 +4251,13 @@ rte_eth_dev_mac_addr_remove(uint16_t port_id, struct rte_ether_addr *addr)
>>   	int index;
>>   
>>   	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
>> +
>> +	if (addr == NULL) {
>> +		RTE_ETHDEV_LOG(ERR, "Failed to remove ethdev port %u MAC address by NULL\n",
>> +			port_id);
>> +		return -EINVAL;
>> +	}
>> +
>>   	dev = &rte_eth_devices[port_id];
>>   	RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->mac_addr_remove, -ENOTSUP);
>>   
>> @@ -4109,6 +4290,12 @@ rte_eth_dev_default_mac_addr_set(uint16_t port_id, struct rte_ether_addr *addr)
>>   
>>   	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
>>   
>> +	if (addr == NULL) {
>> +		RTE_ETHDEV_LOG(ERR, "Failed to set ethdev port %u default MAC address to NULL\n",
>> +			port_id);
>> +		return -EINVAL;
>> +	}
>> +
>>   	if (!rte_is_valid_assigned_ether_addr(addr))
>>   		return -EINVAL;
>>   
>> @@ -4164,6 +4351,12 @@ rte_eth_dev_uc_hash_table_set(uint16_t port_id, struct rte_ether_addr *addr,
>>   
>>   	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
>>   
>> +	if (addr == NULL) {
>> +		RTE_ETHDEV_LOG(ERR, "Failed to set ethdev port %u uc hash table to NULL\n",
>> +			port_id);
>> +		return -EINVAL;
>> +	}
>> +
>>   	dev = &rte_eth_devices[port_id];
>>   	if (rte_is_zero_ether_addr(addr)) {
>>   		RTE_ETHDEV_LOG(ERR, "Port %u: Cannot add NULL MAC address\n",
>> @@ -4265,6 +4458,13 @@ rte_eth_mirror_rule_set(uint16_t port_id,
>>   	struct rte_eth_dev *dev;
>>   
>>   	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
>> +
>> +	if (mirror_conf == NULL) {
>> +		RTE_ETHDEV_LOG(ERR, "Failed to set ethdev port %u mirror rule to NULL\n",
>> +			port_id);
>> +		return -EINVAL;
>> +	}
>> +
>>   	if (mirror_conf->rule_type == 0) {
>>   		RTE_ETHDEV_LOG(ERR, "Mirror rule type can not be 0\n");
>>   		return -EINVAL;
>> @@ -5208,6 +5408,13 @@ rte_eth_timesync_read_rx_timestamp(uint16_t port_id, struct timespec *timestamp,
>>   	struct rte_eth_dev *dev;
>>   
>>   	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
>> +
>> +	if (timestamp == NULL) {
>> +		RTE_ETHDEV_LOG(ERR, "Failed to read ethdev port %u Rx timestamp by NULL\n",
>> +			port_id);
>> +		return -EINVAL;
>> +	}
>> +
>>   	dev = &rte_eth_devices[port_id];
>>   
>>   	RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->timesync_read_rx_timestamp, -ENOTSUP);
>> @@ -5222,6 +5429,13 @@ rte_eth_timesync_read_tx_timestamp(uint16_t port_id,
>>   	struct rte_eth_dev *dev;
>>   
>>   	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
>> +
>> +	if (timestamp == NULL) {
>> +		RTE_ETHDEV_LOG(ERR, "Failed to read ethdev port %u Tx timestamp by NULL\n",
>> +			port_id);
>> +		return -EINVAL;
>> +	}
>> +
>>   	dev = &rte_eth_devices[port_id];
>>   
>>   	RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->timesync_read_tx_timestamp, -ENOTSUP);
>> @@ -5248,6 +5462,13 @@ rte_eth_timesync_read_time(uint16_t port_id, struct timespec *timestamp)
>>   	struct rte_eth_dev *dev;
>>   
>>   	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
>> +
>> +	if (timestamp == NULL) {
>> +		RTE_ETHDEV_LOG(ERR, "Failed to read ethdev port %u time by NULL\n",
>> +			port_id);
>> +		return -EINVAL;
>> +	}
>> +
>>   	dev = &rte_eth_devices[port_id];
>>   
>>   	RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->timesync_read_time, -ENOTSUP);
>> @@ -5261,6 +5482,13 @@ rte_eth_timesync_write_time(uint16_t port_id, const struct timespec *timestamp)
>>   	struct rte_eth_dev *dev;
>>   
>>   	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
>> +
>> +	if (timestamp == NULL) {
>> +		RTE_ETHDEV_LOG(ERR, "Failed to write ethdev port %u time to NULL\n",
>> +			port_id);
>> +		return -EINVAL;
>> +	}
>> +
>>   	dev = &rte_eth_devices[port_id];
>>   
>>   	RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->timesync_write_time, -ENOTSUP);
>> @@ -5274,6 +5502,13 @@ rte_eth_read_clock(uint16_t port_id, uint64_t *clock)
>>   	struct rte_eth_dev *dev;
>>   
>>   	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
>> +
>> +	if (clock == NULL) {
>> +		RTE_ETHDEV_LOG(ERR, "Failed to read ethdev port %u clock by NULL\n",
>> +			port_id);
>> +		return -EINVAL;
>> +	}
>> +
>>   	dev = &rte_eth_devices[port_id];
>>   
>>   	RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->read_clock, -ENOTSUP);
>> @@ -5372,6 +5607,12 @@ rte_eth_dev_get_dcb_info(uint16_t port_id,
>>   
>>   	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
>>   
>> +	if (dcb_info == NULL) {
>> +		RTE_ETHDEV_LOG(ERR, "Failed to get ethdev port %u dcb info by NULL\n",
>> +			port_id);
>> +		return -EINVAL;
>> +	}
>> +
>>   	dev = &rte_eth_devices[port_id];
>>   	memset(dcb_info, 0, sizeof(struct rte_eth_dcb_info));
>>   
>> @@ -5423,6 +5664,12 @@ rte_eth_dev_hairpin_capability_get(uint16_t port_id,
>>   
>>   	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
>>   
>> +	if (cap == NULL) {
>> +		RTE_ETHDEV_LOG(ERR, "Failed to get ethdev port %u hairpin capability by NULL\n",
>> +			port_id);
>> +		return -EINVAL;
>> +	}
>> +
>>   	dev = &rte_eth_devices[port_id];
>>   	RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->hairpin_cap_get, -ENOTSUP);
>>   	memset(cap, 0, sizeof(*cap));
>> @@ -5629,6 +5876,11 @@ rte_eth_representor_id_get(const struct rte_eth_dev *ethdev,
>>   	struct rte_eth_representor_info *info = NULL;
>>   	size_t size;
>>   
>> +	if (ethdev == NULL) {
>> +		RTE_ETHDEV_LOG(ERR, "Failed to get device representor info from NULL\n");
>> +		return -EINVAL;
>> +	}
>> +
>>   	if (type == RTE_ETH_REPRESENTOR_NONE)
>>   		return 0;
>>   	if (repr_id == NULL)
>> diff --git a/lib/librte_ethdev/rte_ethdev.h b/lib/librte_ethdev/rte_ethdev.h
>> index 3b773b6..c1e5d4b 100644
>> --- a/lib/librte_ethdev/rte_ethdev.h
>> +++ b/lib/librte_ethdev/rte_ethdev.h
>> @@ -2702,6 +2702,7 @@ int rte_eth_allmulticast_get(uint16_t port_id);
>>    *   - (0) if successful.
>>    *   - (-ENOTSUP) if the function is not supported in PMD driver.
>>    *   - (-ENODEV) if *port_id* invalid.
>> + *   - (-EINVAL) if bad parameter.
>>    */
>>   int rte_eth_link_get(uint16_t port_id, struct rte_eth_link *link);
>>   
>> @@ -2717,6 +2718,7 @@ int rte_eth_link_get(uint16_t port_id, struct rte_eth_link *link);
>>    *   - (0) if successful.
>>    *   - (-ENOTSUP) if the function is not supported in PMD driver.
>>    *   - (-ENODEV) if *port_id* invalid.
>> + *   - (-EINVAL) if bad parameter.
>>    */
>>   int rte_eth_link_get_nowait(uint16_t port_id, struct rte_eth_link *link);
>>   
>> @@ -2752,7 +2754,7 @@ const char *rte_eth_link_speed_to_str(uint32_t link_speed);
>>    * @param eth_link
>>    *   Link status returned by rte_eth_link_get function
>>    * @return
>> - *   Number of bytes written to str array.
>> + *   Number of bytes written to str array or -EINVAL if bad parameter.
>>    */
>>   __rte_experimental
>>   int rte_eth_link_to_str(char *str, size_t len,
>> @@ -2997,6 +2999,7 @@ int rte_eth_dev_set_rx_queue_stats_mapping(uint16_t port_id,
>>    * @return
>>    *   - (0) if successful
>>    *   - (-ENODEV) if *port_id* invalid.
>> + *   - (-EINVAL) if bad parameter.
>>    */
>>   int rte_eth_macaddr_get(uint16_t port_id, struct rte_ether_addr *mac_addr);
>>   
>> @@ -3041,6 +3044,7 @@ int rte_eth_macaddr_get(uint16_t port_id, struct rte_ether_addr *mac_addr);
>>    *   - (0) if successful.
>>    *   - (-ENOTSUP) if support for dev_infos_get() does not exist for the device.
>>    *   - (-ENODEV) if *port_id* invalid.
>> + *   - (-EINVAL) if bad parameter.
>>    */
>>   int rte_eth_dev_info_get(uint16_t port_id, struct rte_eth_dev_info *dev_info);
>>   
>> @@ -3060,6 +3064,7 @@ int rte_eth_dev_info_get(uint16_t port_id, struct rte_eth_dev_info *dev_info);
>>    *   - (-ENOTSUP) if operation is not supported.
>>    *   - (-ENODEV) if *port_id* invalid.
>>    *   - (-EIO) if device is removed.
>> + *   - (-EINVAL) if bad parameter.
>>    *   - (>0) if *fw_size* is not enough to store firmware version, return
>>    *          the size of the non truncated string.
>>    */
>> @@ -3103,6 +3108,7 @@ int rte_eth_dev_fw_version_get(uint16_t port_id,
>>    *           only num entries will be filled into the ptypes array, but the full
>>    *           count of supported ptypes will be returned.
>>    *   - (-ENODEV) if *port_id* invalid.
>> + *   - (-EINVAL) if bad parameter.
>>    */
>>   int rte_eth_dev_get_supported_ptypes(uint16_t port_id, uint32_t ptype_mask,
>>   				     uint32_t *ptypes, int num);
>> @@ -3153,6 +3159,7 @@ int rte_eth_dev_set_ptypes(uint16_t port_id, uint32_t ptype_mask,
>>    * @return
>>    *   - (0) if successful.
>>    *   - (-ENODEV) if *port_id* invalid.
>> + *   - (-EINVAL) if bad parameter.
>>    */
>>   int rte_eth_dev_get_mtu(uint16_t port_id, uint16_t *mtu);
>>   
>> @@ -3347,7 +3354,7 @@ rte_eth_tx_buffer_init(struct rte_eth_dev_tx_buffer *buffer, uint16_t size);
>>    * @param userdata
>>    *   Arbitrary parameter to be passed to the callback function
>>    * @return
>> - *   0 on success, or -1 on error with rte_errno set appropriately
>> + *   0 on success, or -EINVAL if bad parameter
>>    */
>>   int
>>   rte_eth_tx_buffer_set_err_callback(struct rte_eth_dev_tx_buffer *buffer,
>> @@ -3774,6 +3781,7 @@ int rte_eth_fec_set(uint16_t port_id, uint32_t fec_capa);
>>    *   - (-ENOTSUP) if hardware doesn't support flow control.
>>    *   - (-ENODEV)  if *port_id* invalid.
>>    *   - (-EIO)  if device is removed.
>> + *   - (-EINVAL) if bad parameter.
>>    */
>>   int rte_eth_dev_flow_ctrl_get(uint16_t port_id,
>>   			      struct rte_eth_fc_conf *fc_conf);
>> @@ -3845,7 +3853,8 @@ int rte_eth_dev_mac_addr_add(uint16_t port_id, struct rte_ether_addr *mac_addr,
>>    *   - (0) if successful, or *mac_addr* didn't exist.
>>    *   - (-ENOTSUP) if hardware doesn't support.
>>    *   - (-ENODEV) if *port* invalid.
>> - *   - (-EADDRINUSE) if attempting to remove the default MAC address
>> + *   - (-EADDRINUSE) if attempting to remove the default MAC address.
>> + *   - (-EINVAL) if MAC address is invalid.
>>    */
>>   int rte_eth_dev_mac_addr_remove(uint16_t port_id,
>>   				struct rte_ether_addr *mac_addr);
>> @@ -4044,6 +4053,7 @@ int rte_eth_dev_rss_hash_update(uint16_t port_id,
>>    *   - (-ENODEV) if port identifier is invalid.
>>    *   - (-EIO) if device is removed.
>>    *   - (-ENOTSUP) if hardware doesn't support RSS.
>> + *   - (-EINVAL) if bad parameter.
>>    */
>>   int
>>   rte_eth_dev_rss_hash_conf_get(uint16_t port_id,
>> @@ -4112,6 +4122,7 @@ rte_eth_dev_udp_tunnel_port_delete(uint16_t port_id,
>>    *   - (-ENODEV) if port identifier is invalid.
>>    *   - (-EIO) if device is removed.
>>    *   - (-ENOTSUP) if hardware doesn't support.
>> + *   - (-EINVAL) if bad parameter.
>>    */
>>   int rte_eth_dev_get_dcb_info(uint16_t port_id,
>>   			     struct rte_eth_dcb_info *dcb_info);
>> @@ -4628,6 +4639,7 @@ int rte_eth_timesync_adjust_time(uint16_t port_id, int64_t delta);
>>    *
>>    * @return
>>    *   - 0: Success.
>> + *   - -EINVAL: Bad parameter.
>>    */
>>   int rte_eth_timesync_read_time(uint16_t port_id, struct timespec *time);
>>   
>> @@ -4694,6 +4706,7 @@ int rte_eth_timesync_write_time(uint16_t port_id, const struct timespec *time);
>>    *   - 0: Success.
>>    *   - -ENODEV: The port ID is invalid.
>>    *   - -ENOTSUP: The function is not supported by the Ethernet driver.
>> + *   - -EINVAL: if bad parameter.
>>    */
>>   __rte_experimental
>>   int
>> @@ -4797,6 +4810,7 @@ rte_eth_dev_get_sec_ctx(uint16_t port_id);
>>    * @return
>>    *   - (0) if successful.
>>    *   - (-ENOTSUP) if hardware doesn't support.
>> + *   - (-EINVAL) if bad parameter.
>>    */
>>   __rte_experimental
>>   int rte_eth_dev_hairpin_capability_get(uint16_t port_id,
>>
> 
> .
> 


More information about the dev mailing list