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

[Pascal Mazon <pascal.mazon@6wind.com>]

Hi,

Can you also not use a global value for user_mac, but instead change the
last argument for eth_dev_tap_create():
Use directly a char mac[ETHER_ADDR_LEN], automatic variable from
rte_pmd_tap_probe().
In set_mac_type(), you can check either for "fixed" or a correct custom
mac address.
Then eth_dev_tap_create() can check if the provided mac is empty (!fixed
and !custom_mac), to generate a random one.

/* desired format aa:bb:cc:dd:ee:ff:11 */      The :11 goes beyond
standard MAC addresses ;-)

The commit log has few mistakes:
- missing "to" after "applications"
- "argumentinfrastructure"

I otherwise concur with Ferruh's remarks.

Best regards,
Pascal

On 07/12/2017 21:11, Ferruh Yigit wrote:
> 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;
>>  }
>>  
>>