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.vargh...@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;
>> }
>>
>>
