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

["Varghese, Vipin" <vipin.varghese@intel.com>]

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 <pascal.mazon@6wind.com>; dev@dpdk.org
> Cc: Yigit, Ferruh <ferruh.yigit@intel.com>; Jain, Deepak K
> <deepak.k.jain@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.mazon@6wind.com]
> > Sent: Friday, February 2, 2018 9:16 AM
> > To: Varghese, Vipin <vipin.varghese@intel.com>; dev@dpdk.org
> > Cc: Yigit, Ferruh <ferruh.yigit@intel.com>; Jain, Deepak K
> > <deepak.k.jain@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>