[dpdk-dev] net/tap: allow user MAC to be passed as args
Checks
Commit Message
Allow TAP PMD to pass user desired MAC address as argument.
The argument value is processed as string, where each 2 bytes
are converted to HEX MAC address after validation.
Signed-off-by: Vipin Varghese <vipin.varghese@intel.com>
---
doc/guides/nics/tap.rst | 6 +++++
drivers/net/tap/rte_eth_tap.c | 62 +++++++++++++++++++++++++++++++++++++++----
2 files changed, 63 insertions(+), 5 deletions(-)
Comments
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.
Additional comments inline.
Best regards,
Pascal
On 31/01/2018 19:22, Vipin Varghese wrote:
> Allow TAP PMD to pass user desired MAC address as argument.
> The argument value is processed as string, where each 2 bytes
> are converted to HEX MAC address after validation.
>
> Signed-off-by: Vipin Varghese <vipin.varghese@intel.com>
> ---
> doc/guides/nics/tap.rst | 6 +++++
> drivers/net/tap/rte_eth_tap.c | 62 +++++++++++++++++++++++++++++++++++++++----
> 2 files changed, 63 insertions(+), 5 deletions(-)
>
> diff --git a/doc/guides/nics/tap.rst b/doc/guides/nics/tap.rst
> index dc6f834..6b083c8 100644
> --- a/doc/guides/nics/tap.rst
> +++ b/doc/guides/nics/tap.rst
> @@ -69,6 +69,12 @@ for each interface string containing ``mac=fixed``. The MAC address is formatted
> as 00:'d':'t':'a':'p':[00-FF]. Convert the characters to hex and you get the
> actual MAC address: ``00:64:74:61:70:[00-FF]``.
>
> + --vdev=net_tap0,mac="00:64:74:61:70:11"
> +
> +The MAC address will have a user value passed as string. The MAC address is in
> +format with delimeter ``:``. The string is byte converted to hex and you get
> +the actual MAC address: ``00:64:74:61:70:11``.
> +
> It is possible to specify a remote netdevice to capture packets from by adding
> ``remote=foo1``, for example::
>
> diff --git a/drivers/net/tap/rte_eth_tap.c b/drivers/net/tap/rte_eth_tap.c
> index 29d6356..3489b04 100644
> --- a/drivers/net/tap/rte_eth_tap.c
> +++ b/drivers/net/tap/rte_eth_tap.c
> @@ -49,7 +49,14 @@
> #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:"
> + pmd->name,
> + user_mac[0], user_mac[1], user_mac[2],
> + user_mac[3], user_mac[4], user_mac[5]);
> + rte_memcpy(&pmd->eth_addr, user_mac, ETHER_ADDR_LEN);
> } else {
> eth_random_addr((uint8_t *)&pmd->eth_addr);
> }
> @@ -1577,10 +1591,48 @@ enum ioctl_mode {
> const char *value,
> void *extra_args)
> {
> - if (value &&
> - !strncasecmp(ETH_TAP_MAC_FIXED, value, strlen(ETH_TAP_MAC_FIXED)))
> - *(int *)extra_args = 1;
> + char mac_temp[20] = {0}, *mac_byte = NULL;
Instead of hardcoded values, I'd use
mac_temp[strlen(ETH_TAP_USR_MAC_FMT) + 1]
> + unsigned int index = 0;
> +
> + if (!value)
> + return 0;
> +
> + if (!strncasecmp(ETH_TAP_MAC_FIXED, value,
> + strlen(ETH_TAP_MAC_FIXED))) {
> + *(int *)extra_args = ETH_TAP_MAC_STR_FXD;
> + goto success;
> + }
> +
> + if (strlen(value) == 17) {
And here 17 => strlen(ETH_TAP_USR_MAC_FMT)
> + 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.
> + mac_byte = strtok(mac_temp, ":");
> +
> + while ((mac_byte != NULL) &&
> + strspn(mac_byte, ETH_TAP_CMP_MAC_FMT) &&
> + strspn((mac_byte + 1), ETH_TAP_CMP_MAC_FMT) &&
> + strlen(mac_byte) == 2) {
> + user_mac[index] = strtoul(mac_byte, NULL, 16);
> + mac_byte = strtok(NULL, ":");
> + index += 1;
> + }
> +
> + if (index != 6)
> + goto error;
> +
> + *(int *)extra_args = ETH_TAP_MAC_STR_USR;
> + } else {
> + goto error;
> + }
> +
> +success:
> + RTE_LOG(DEBUG, PMD, "TAP user MAC param (%s)\n", value);
> return 0;
> +
> +error:
> + RTE_LOG(ERR, PMD, "TAP user MAC (%s) is not in format (%s|%s)\n",
> + value, ETH_TAP_MAC_FIXED, ETH_TAP_USR_MAC_FMT);
> + return -1;
> }
>
> /* Open a TAP interface device.
> @@ -1716,5 +1768,5 @@ enum ioctl_mode {
> RTE_PMD_REGISTER_PARAM_STRING(net_tap,
> ETH_TAP_IFACE_ARG "=<string> "
> ETH_TAP_SPEED_ARG "=<int> "
> - ETH_TAP_MAC_ARG "=" ETH_TAP_MAC_FIXED " "
> + ETH_TAP_MAC_ARG "=" ETH_TAP_MAC_ARG_FMT
> ETH_TAP_REMOTE_ARG "=<string>");
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>
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>
@@ -69,6 +69,12 @@ for each interface string containing ``mac=fixed``. The MAC address is formatted
as 00:'d':'t':'a':'p':[00-FF]. Convert the characters to hex and you get the
actual MAC address: ``00:64:74:61:70:[00-FF]``.
+ --vdev=net_tap0,mac="00:64:74:61:70:11"
+
+The MAC address will have a user value passed as string. The MAC address is in
+format with delimeter ``:``. The string is byte converted to hex and you get
+the actual MAC address: ``00:64:74:61:70:11``.
+
It is possible to specify a remote netdevice to capture packets from by adding
``remote=foo1``, for example::
@@ -49,7 +49,14 @@
#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",
+ pmd->name,
+ user_mac[0], user_mac[1], user_mac[2],
+ user_mac[3], user_mac[4], user_mac[5]);
+ rte_memcpy(&pmd->eth_addr, user_mac, ETHER_ADDR_LEN);
} else {
eth_random_addr((uint8_t *)&pmd->eth_addr);
}
@@ -1577,10 +1591,48 @@ enum ioctl_mode {
const char *value,
void *extra_args)
{
- if (value &&
- !strncasecmp(ETH_TAP_MAC_FIXED, value, strlen(ETH_TAP_MAC_FIXED)))
- *(int *)extra_args = 1;
+ char mac_temp[20] = {0}, *mac_byte = NULL;
+ unsigned int index = 0;
+
+ if (!value)
+ return 0;
+
+ if (!strncasecmp(ETH_TAP_MAC_FIXED, value,
+ strlen(ETH_TAP_MAC_FIXED))) {
+ *(int *)extra_args = ETH_TAP_MAC_STR_FXD;
+ goto success;
+ }
+
+ if (strlen(value) == 17) {
+ strncpy(mac_temp, value, 18);
+ mac_temp[19] = '\0';
+ mac_byte = strtok(mac_temp, ":");
+
+ while ((mac_byte != NULL) &&
+ strspn(mac_byte, ETH_TAP_CMP_MAC_FMT) &&
+ strspn((mac_byte + 1), ETH_TAP_CMP_MAC_FMT) &&
+ strlen(mac_byte) == 2) {
+ user_mac[index] = strtoul(mac_byte, NULL, 16);
+ mac_byte = strtok(NULL, ":");
+ index += 1;
+ }
+
+ if (index != 6)
+ goto error;
+
+ *(int *)extra_args = ETH_TAP_MAC_STR_USR;
+ } else {
+ goto error;
+ }
+
+success:
+ RTE_LOG(DEBUG, PMD, "TAP user MAC param (%s)\n", value);
return 0;
+
+error:
+ RTE_LOG(ERR, PMD, "TAP user MAC (%s) is not in format (%s|%s)\n",
+ value, ETH_TAP_MAC_FIXED, ETH_TAP_USR_MAC_FMT);
+ return -1;
}
/* Open a TAP interface device.
@@ -1716,5 +1768,5 @@ enum ioctl_mode {
RTE_PMD_REGISTER_PARAM_STRING(net_tap,
ETH_TAP_IFACE_ARG "=<string> "
ETH_TAP_SPEED_ARG "=<int> "
- ETH_TAP_MAC_ARG "=" ETH_TAP_MAC_FIXED " "
+ ETH_TAP_MAC_ARG "=" ETH_TAP_MAC_ARG_FMT
ETH_TAP_REMOTE_ARG "=<string>");