[dpdk-dev] [PATCH] net/tap: allow user MAC to be passed as args
Varghese, Vipin
vipin.varghese at intel.com
Fri Feb 2 10:49:38 CET 2018
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 at 6wind.com]
> Sent: Friday, February 2, 2018 9:16 AM
> To: Varghese, Vipin <vipin.varghese at intel.com>; dev at dpdk.org
> Cc: Yigit, Ferruh <ferruh.yigit at intel.com>; Jain, Deepak K
> <deepak.k.jain at 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>
More information about the dev
mailing list