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-boun...@dpdk.org] On Behalf Of Varghese, Vipin > Sent: Friday, February 2, 2018 9:50 AM > To: Pascal Mazon <pascal.ma...@6wind.com>; dev@dpdk.org > Cc: Yigit, Ferruh <ferruh.yi...@intel.com>; Jain, Deepak K > <deepak.k.j...@intel.com> > 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 > > > -----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>
