Re: [dpdk-dev] [PATCH v1] net/tap: allow user mac to be passed as args

[Ferruh Yigit <ferruh.yigit@intel.com>]

On 11/30/2017 11:49 AM, Vipin Varghese wrote:
> One of the uses cases from customer site is use TAP PMD to intake
> user specific MAC address during probe. This allows applications
> make use of interfaces with desired MAC.
> 
> Extending MAC argumentinfrastructure for tap PMD; we pass custom
> MAC address in string format (sample - 11:22:33:44:55:66).

Overall lgtm, please check a few comments below.

> 
> Signed-off-by: Vipin Varghese <vipin.varghese@intel.com>
> ---
>  drivers/net/tap/rte_eth_tap.c | 56 +++++++++++++++++++++++++++++++++++++++----
>  1 file changed, 52 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/net/tap/rte_eth_tap.c b/drivers/net/tap/rte_eth_tap.c
> index 6b27679..0c53458 100644
> --- a/drivers/net/tap/rte_eth_tap.c
> +++ b/drivers/net/tap/rte_eth_tap.c
> @@ -81,6 +81,8 @@
>  #define FLOWER_KERNEL_VERSION KERNEL_VERSION(4, 2, 0)
>  #define FLOWER_VLAN_KERNEL_VERSION KERNEL_VERSION(4, 9, 0)
>  
> +static unsigned char user_mac[ETHER_ADDR_LEN];
> +
>  static struct rte_vdev_driver pmd_tap_drv;
>  
>  static const char *valid_arguments[] = {
> @@ -1291,13 +1293,20 @@ enum ioctl_mode {
>  		pmd->txq[i].fd = -1;
>  	}
>  
> -	if (fixed_mac_type) {
> +	if (fixed_mac_type == 1) {

Instead of hardcoded type values 1 & 2, can you please use macros?

>  		/* fixed mac = 00:64:74:61:70:<iface_idx> */
>  		static int iface_idx;
>  		char mac[ETHER_ADDR_LEN] = "\0dtap";
>  
>  		mac[ETHER_ADDR_LEN - 1] = iface_idx++;
>  		rte_memcpy(&pmd->eth_addr, mac, ETHER_ADDR_LEN);
> +	} else if (fixed_mac_type == 2) {
> +		/* user mac is recieved */

s/recieved/received

> +		RTE_LOG(INFO, PMD,
> +			"Using user MAC (%02x:%02x:%02x:%02x:%02x:%02x)\n",
> +			user_mac[0], user_mac[1], user_mac[2],
> +			user_mac[3], user_mac[4], user_mac[5]);
> +		rte_memcpy(&pmd->eth_addr, user_mac, ETHER_ADDR_LEN);
>  	} else {
>  		eth_random_addr((uint8_t *)&pmd->eth_addr);
>  	}
> @@ -1471,9 +1480,48 @@ enum ioctl_mode {
>  	     const char *value,
>  	     void *extra_args)
>  {
> -	if (value &&
> -	    !strncasecmp(ETH_TAP_MAC_FIXED, value, strlen(ETH_TAP_MAC_FIXED)))
> -		*(int *)extra_args = 1;
> +	char macTemp[20], *macByte = NULL;
> +	unsigned int index = 0;
> +
> +	if (value) {
> +		if (!strncasecmp(ETH_TAP_MAC_FIXED, value,
> +			strlen(ETH_TAP_MAC_FIXED))) {
> +			*(int *)extra_args = 1;
> +		} else {
> +			RTE_LOG(INFO, PMD, "User shared MAC param (%s)\n",
> +				value);

Is this log needs to be in INFO level, since there is already and info level log
when MAC set, what about making this debug?

> +
> +			/* desired format aa:bb:cc:dd:ee:ff:11 */

Can you please update RTE_PMD_REGISTER_PARAM_STRING with new param, put expected
format info there as well?

> +			if (strlen(value) == 17) {
> +				strncpy(macTemp, value, 18);
> +
> +				macByte = strtok(macTemp, ":");
> +				while ((macByte != NULL) &&
> +					(strspn(macByte, "0123456789ABCDEFabcdef")) &&
> +					(strspn((macByte + 1), "0123456789ABCDEFabcdef")) &&
> +					(strlen(macByte) == 2)) {
> +					user_mac[index] = strtoul(macByte, NULL, 16);
> +					macByte = strtok(NULL, ":");
> +					index += 1;
> +				}

I would extract the string to mac logic into its own function, but up to you.

> +
> +				if (index != 6) {
> +					RTE_LOG(ERR, PMD, " failure in MAC value (%s) at %u\n",
> +						macByte, index + 1);
> +					return -1;

And this is not related to this patch, but just as reminder, when a virtual
driver probe fails vdev bus stops probing with an error, so all remaining
virtual devices are not probed, in case one might want to fix ;)

> +				}
> +
> +				RTE_LOG(DEBUG, PMD, " User MAC (%s) considered\n",

"User defined MAC" ? And can you please add an port identifier, port_id or
device name or interface name, for the case there are multiple tap device to
identify which one get user defined MAC.

> +					value);
> +				*(int *)extra_args = 2;
> +			} else {
> +				RTE_LOG(ERR, PMD, " Mismatch on format for (%s)\n",
> +					value);
> +				return -1;
> +			}
> +		}
> +	}
> +
>  	return 0;
>  }
>  
>