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

Hi Pascal,

Waiting for your inputs in changing the function argument from int32_t to uint64_t.

Thanks
Vipin Varghese

-----Original Message-----
From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Varghese, Vipin
Sent: Friday, February 2, 2018 9:50 AM
To: Pascal Mazon <redacted>; dev@dpdk.org
Cc: Yigit, Ferruh <redacted>; Jain, Deepak K
[off-list ref]
Subject: Re: [dpdk-dev] [PATCH] net/tap: allow user MAC to be passed as args

Hi Pascal,

Sincere apologizes, I think I missed out since rework was asked. Please find my
answers inline to the comment

quoted

-----Original Message-----
From: Pascal Mazon [mailto:pascal.mazon@6wind.com]
Sent: Friday, February 2, 2018 9:16 AM
To: Varghese, Vipin <redacted>; dev@dpdk.org
Cc: Yigit, Ferruh <redacted>; Jain, Deepak K
[off-list ref]
Subject: Re: [PATCH] net/tap: allow user MAC to be passed as args

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.

Last argument for eth_dev_tap_create is ' int fixed_mac_type '. Would like me
to change this to 'uint64_t fixed_mac_type' to accommodate the MAC address?

Note: Should we change the API arguments?

quoted

Additional comments inline.

Best regards,
Pascal

On 31/01/2018 19:22, Vipin Varghese wrote:

<Snipped>

quoted

quoted

 #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"]"

quoted

+
 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:"

Ok, I can make this change.

<Snipped>

quoted

quoted

+	char mac_temp[20] = {0}, *mac_byte = NULL;

Instead of hardcoded values, I'd use
mac_temp[strlen(ETH_TAP_USR_MAC_FMT) + 1]

Ok, I can make this change.

<Snipped>

quoted

quoted

+
+	if (strlen(value) == 17) {

And here 17 => strlen(ETH_TAP_USR_MAC_FMT)

Ok

quoted

quoted

+		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.

OK, I will check the same.

quoted

quoted

+		mac_byte = strtok(mac_temp, ":");

<Snipped>