Hi Pascal, Sincere apologizes, I think I missed out since rework was asked. Please find my answers inline to the comment
> -----Original Message----- > From: Pascal Mazon [mailto:pascal.ma...@6wind.com] > Sent: Friday, February 2, 2018 9:16 AM > To: Varghese, Vipin <vipin.vargh...@intel.com>; dev@dpdk.org > Cc: Yigit, Ferruh <ferruh.yi...@intel.com>; Jain, Deepak K > <deepak.k.j...@intel.com> > 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? > > Additional comments inline. > > Best regards, > Pascal > > On 31/01/2018 19:22, Vipin Varghese wrote: <Snipped> > > #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:" Ok, I can make this change. <Snipped> > > + 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> > > + > > + if (strlen(value) == 17) { > And here 17 => strlen(ETH_TAP_USR_MAC_FMT) Ok > > + 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. > > + mac_byte = strtok(mac_temp, ":"); <Snipped>
