Re: [PATCH v1] net/tap: allow user mac to be passed as args
From: Varghese, Vipin <hidden>
Date: 2017-12-16 02:21:23
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.
quoted hunk ↗ jump to hunk
Signed-off-by: Vipin Varghese <redacted> --- drivers/net/tap/rte_eth_tap.c | 56 +++++++++++++++++++++++++++++++++++++++---- 1 file changed, 52 insertions(+), 4 deletions(-)diff --git a/drivers/net/tap/rte_eth_tap.cb/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) #defineFLOWER_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?
quoted
Ok, Adding MACROs MAC_STRING_NULL, MAC_STRING_FIXED and MAC_STRING_USER
/* 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
quoted
Ok
quoted hunk ↗ jump to hunk
+ 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?
quoted
Ok
+ + /* 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?
quoted
Ok
+ 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.
quoted
not done
+
+ 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. VV> not done, each device in DPDK rte_eal_init is sequentially scanned and probed, hence log starts with 'iface,[parameters]', then probe, create and init is called for TAP PMD one after another. With minimalistic logging style the current format is kept.
+ value);
+ *(int *)extra_args = 2;
+ } else {
+ RTE_LOG(ERR, PMD, " Mismatch on format for (%s)\n",
+ value);
+ return -1;
+ }
+ }
+ }
+
return 0;
}