DPDK patches and discussions
 help / color / mirror / Atom feed
From: "Varghese, Vipin" <vipin.varghese@intel.com>
To: Pascal Mazon <pascal.mazon@6wind.com>, "dev@dpdk.org" <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
Date: Fri, 2 Feb 2018 09:49:38 +0000	[thread overview]
Message-ID: <4C9E0AB70F954A408CC4ADDBF0F8FA7D4D193FFA@BGSMSX101.gar.corp.intel.com> (raw)
In-Reply-To: <021c3202-fa37-ba37-fc7d-96eb62e4ff95@6wind.com>

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>

  reply	other threads:[~2018-02-02  9:49 UTC|newest]

Thread overview: 19+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-01-31 18:22 Vipin Varghese
2018-02-02  9:16 ` Pascal Mazon
2018-02-02  9:49   ` Varghese, Vipin [this message]
2018-02-05  8:23     ` Varghese, Vipin
2018-02-05 15:54 ` [dpdk-dev] [PATCH v2] " Pascal Mazon
2018-02-05 15:58   ` Varghese, Vipin
2018-02-05 18:13     ` Varghese, Vipin
2018-02-05 18:42   ` Ferruh Yigit
2018-02-12 14:44   ` [dpdk-dev] [PATCH v1] " Vipin Varghese
2018-02-22 11:52     ` Pascal Mazon
2018-03-06 16:41     ` Ferruh Yigit
2018-03-07  9:44       ` Varghese, Vipin
2018-03-12 20:21     ` [dpdk-dev] [PATCH v2] " Vipin Varghese
2018-03-12 15:08       ` Ferruh Yigit
2018-03-12 15:38         ` Varghese, Vipin
2018-03-12 17:33       ` Stephen Hemminger
2018-03-12 21:53       ` [dpdk-dev] [PATCH v3] " Vipin Varghese
2018-03-12 17:45         ` Ferruh Yigit
2018-03-16 14:22           ` Ferruh Yigit

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=4C9E0AB70F954A408CC4ADDBF0F8FA7D4D193FFA@BGSMSX101.gar.corp.intel.com \
    --to=vipin.varghese@intel.com \
    --cc=deepak.k.jain@intel.com \
    --cc=dev@dpdk.org \
    --cc=ferruh.yigit@intel.com \
    --cc=pascal.mazon@6wind.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).