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

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

Hi,

You didn't address my request about not using a global value. Was there
a good reason?

I paste it here again as a reminder:

  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.

Additional comments inline.

Best regards,
Pascal

On 31/01/2018 19:22, Vipin Varghese wrote:
> Allow TAP PMD to pass user desired MAC address as argument.
> The argument value is processed as string, where each 2 bytes
> are converted to HEX MAC address after validation.
>
> Signed-off-by: Vipin Varghese <vipin.varghese@intel.com>
> ---
>  doc/guides/nics/tap.rst       |  6 +++++
>  drivers/net/tap/rte_eth_tap.c | 62 +++++++++++++++++++++++++++++++++++++++----
>  2 files changed, 63 insertions(+), 5 deletions(-)
>
> diff --git a/doc/guides/nics/tap.rst b/doc/guides/nics/tap.rst
> index dc6f834..6b083c8 100644
> --- a/doc/guides/nics/tap.rst
> +++ b/doc/guides/nics/tap.rst
> @@ -69,6 +69,12 @@ for each interface string containing ``mac=fixed``. The MAC address is formatted
>  as 00:'d':'t':'a':'p':[00-FF]. Convert the characters to hex and you get the
>  actual MAC address: ``00:64:74:61:70:[00-FF]``.
>  
> +   --vdev=net_tap0,mac="00:64:74:61:70:11"
> +
> +The MAC address will have a user value passed as string. The MAC address is in
> +format with delimeter ``:``. The string is byte converted to hex and you get
> +the actual MAC address: ``00:64:74:61:70:11``.
> +
>  It is possible to specify a remote netdevice to capture packets from by adding
>  ``remote=foo1``, for example::
>  
> diff --git a/drivers/net/tap/rte_eth_tap.c b/drivers/net/tap/rte_eth_tap.c
> index 29d6356..3489b04 100644
> --- a/drivers/net/tap/rte_eth_tap.c
> +++ b/drivers/net/tap/rte_eth_tap.c
> @@ -49,7 +49,14 @@
>  #define ETH_TAP_MAC_ARG         "mac"
>  #define ETH_TAP_MAC_FIXED       "fixed"
>  
> +#define ETH_TAP_MAC_STR_FXD     1
> +#define ETH_TAP_MAC_STR_USR     2
> +#define ETH_TAP_USR_MAC_FMT     "xx:xx:xx:xx:xx:xx"
> +#define ETH_TAP_CMP_MAC_FMT     "0123456789ABCDEFabcdef"
> +#define ETH_TAP_MAC_ARG_FMT     "["ETH_TAP_MAC_FIXED "|" ETH_TAP_USR_MAC_FMT"]"
> +
>  static struct rte_vdev_driver pmd_tap_drv;
> +static unsigned char user_mac[ETHER_ADDR_LEN];
>  
>  static const char *valid_arguments[] = {
>  	ETH_TAP_IFACE_ARG,
> @@ -1397,13 +1404,20 @@ enum ioctl_mode {
>  		pmd->txq[i].fd = -1;
>  	}
>  
> -	if (fixed_mac_type) {
> +	if (fixed_mac_type == ETH_TAP_MAC_STR_FXD) {
>  		/* 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 == ETH_TAP_MAC_STR_USR) {
> +		RTE_LOG(INFO, PMD,
> +			"%s; user MAC (%02x:%02x:%02x:%02x:%02x:%02x) argument\n",
Shouldn't it be a colon there? "%s:"
> +			pmd->name,
> +			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);
>  	}
> @@ -1577,10 +1591,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 mac_temp[20] = {0}, *mac_byte = NULL;
Instead of hardcoded values, I'd use
mac_temp[strlen(ETH_TAP_USR_MAC_FMT) + 1]
> +	unsigned int index = 0;
> +
> +	if (!value)
> +		return 0;
> +
> +	if (!strncasecmp(ETH_TAP_MAC_FIXED, value,
> +			strlen(ETH_TAP_MAC_FIXED))) {
> +		*(int *)extra_args = ETH_TAP_MAC_STR_FXD;
> +		goto success;
> +	}
> +
> +	if (strlen(value) == 17) {
And here 17 => strlen(ETH_TAP_USR_MAC_FMT)
> +		strncpy(mac_temp, value, 18);
> +		mac_temp[19] = '\0';
Instead of those two lines, I'd rather have snprintf(mac_temp,
sizeof(mac_temp), "%s", value).
It handles the trailing \0 nicely.
> +		mac_byte = strtok(mac_temp, ":");
> +
> +		while ((mac_byte != NULL) &&
> +				strspn(mac_byte, ETH_TAP_CMP_MAC_FMT) &&
> +				strspn((mac_byte + 1), ETH_TAP_CMP_MAC_FMT) &&
> +				strlen(mac_byte) == 2) {
> +			user_mac[index] = strtoul(mac_byte, NULL, 16);
> +			mac_byte = strtok(NULL, ":");
> +			index += 1;
> +		}
> +
> +		if (index != 6)
> +			goto error;
> +
> +		*(int *)extra_args = ETH_TAP_MAC_STR_USR;
> +	} else {
> +		goto error;
> +	}
> +
> +success:
> +	RTE_LOG(DEBUG, PMD, "TAP user MAC param (%s)\n", value);
>  	return 0;
> +
> +error:
> +	RTE_LOG(ERR, PMD, "TAP user MAC (%s) is not in format (%s|%s)\n",
> +		value, ETH_TAP_MAC_FIXED, ETH_TAP_USR_MAC_FMT);
> +	return -1;
>  }
>  
>  /* Open a TAP interface device.
> @@ -1716,5 +1768,5 @@ enum ioctl_mode {
>  RTE_PMD_REGISTER_PARAM_STRING(net_tap,
>  			      ETH_TAP_IFACE_ARG "=<string> "
>  			      ETH_TAP_SPEED_ARG "=<int> "
> -			      ETH_TAP_MAC_ARG "=" ETH_TAP_MAC_FIXED " "
> +			      ETH_TAP_MAC_ARG "=" ETH_TAP_MAC_ARG_FMT
>  			      ETH_TAP_REMOTE_ARG "=<string>");