diff mbox

[dpdk-dev] net/tap: fix dev name look-up

Message ID 1ad979ca71ba1ebe10572237a6a5b4694c23b645.1488531158.git.pascal.mazon@6wind.com (mailing list archive)
State Superseded, archived
Delegated to: Ferruh Yigit
Headers

Checks

Context Check Description
ci/checkpatch success coding style OK
ci/Intel-compilation success Compilation OK

Commit Message

Pascal Mazon March 3, 2017, 8:54 a.m. UTC 
  The call to rte_eth_dev_allocate(tap_name) sets dev->data->name to
tap_name (e.g. "dtap0").

A look-up using tap_name is expected to return this device, not a
look-up using name (e.g. "net_tap0").

Signed-off-by: Pascal Mazon <pascal.mazon@6wind.com>
---
 drivers/net/tap/rte_eth_tap.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Wiles, Keith March 3, 2017, 3:19 p.m. UTC | #1 
> On Mar 3, 2017, at 2:54 AM, Pascal Mazon <pascal.mazon@6wind.com> wrote:
> 
> The call to rte_eth_dev_allocate(tap_name) sets dev->data->name to
> tap_name (e.g. "dtap0").
> 
> A look-up using tap_name is expected to return this device, not a
> look-up using name (e.g. "net_tap0").
> 
> Signed-off-by: Pascal Mazon <pascal.mazon@6wind.com>

Acked-by: Keith Wiles <keith.wiles@intel.com>

> ---
> drivers/net/tap/rte_eth_tap.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/net/tap/rte_eth_tap.c b/drivers/net/tap/rte_eth_tap.c
> index 47a706070652..14c345f07afa 100644
> --- a/drivers/net/tap/rte_eth_tap.c
> +++ b/drivers/net/tap/rte_eth_tap.c
> @@ -691,7 +691,7 @@ eth_dev_tap_create(const char *name, char *tap_name)
> 	dev->driver = NULL;
> 	dev->rx_pkt_burst = pmd_rx_burst;
> 	dev->tx_pkt_burst = pmd_tx_burst;
> -	snprintf(dev->data->name, sizeof(dev->data->name), "%s", name);
> +	snprintf(dev->data->name, sizeof(dev->data->name), "%s", tap_name);
> 
> 	/* Presetup the fds to -1 as being not valid */
> 	for (i = 0; i < RTE_PMD_TAP_MAX_QUEUES; i++) {
> -- 
> 2.8.0.rc0
> 

Regards,
Keith
Ferruh Yigit March 5, 2017, 9:35 p.m. UTC | #2 
On 3/3/2017 8:54 AM, Pascal Mazon wrote:
> The call to rte_eth_dev_allocate(tap_name) sets dev->data->name to
> tap_name (e.g. "dtap0").
> 
> A look-up using tap_name is expected to return this device, not a
> look-up using name (e.g. "net_tap0").

This will break rte_pmd_tap_remove(), because it gets device name
(net_tap0) as parameter.

And logically this is wrong too, current eth_dev->data->name is to keep
device name, all other PMDs use this way, not for Linux interface name.

Current tap PMD, first gives "dtap0" to rte_eth_dev_allocate() as
argument, which sets device name to this value. Later with snprintf()
overwrites the device name with correct value, I think better thing to
do is give correct argument to rte_eth_dev_allocate() and remove snprintf()

> 
> Signed-off-by: Pascal Mazon <pascal.mazon@6wind.com>
> ---
>  drivers/net/tap/rte_eth_tap.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/net/tap/rte_eth_tap.c b/drivers/net/tap/rte_eth_tap.c
> index 47a706070652..14c345f07afa 100644
> --- a/drivers/net/tap/rte_eth_tap.c
> +++ b/drivers/net/tap/rte_eth_tap.c
> @@ -691,7 +691,7 @@ eth_dev_tap_create(const char *name, char *tap_name)
>  	dev->driver = NULL;
>  	dev->rx_pkt_burst = pmd_rx_burst;
>  	dev->tx_pkt_burst = pmd_tx_burst;
> -	snprintf(dev->data->name, sizeof(dev->data->name), "%s", name);
> +	snprintf(dev->data->name, sizeof(dev->data->name), "%s", tap_name);
>  
>  	/* Presetup the fds to -1 as being not valid */
>  	for (i = 0; i < RTE_PMD_TAP_MAX_QUEUES; i++) {
>
Wiles, Keith March 6, 2017, 2:15 p.m. UTC | #3 
> On Mar 5, 2017, at 3:35 PM, Yigit, Ferruh <ferruh.yigit@intel.com> wrote:
> 
> On 3/3/2017 8:54 AM, Pascal Mazon wrote:
>> The call to rte_eth_dev_allocate(tap_name) sets dev->data->name to
>> tap_name (e.g. "dtap0").
>> 
>> A look-up using tap_name is expected to return this device, not a
>> look-up using name (e.g. "net_tap0").
> 
> This will break rte_pmd_tap_remove(), because it gets device name
> (net_tap0) as parameter.
> 
> And logically this is wrong too, current eth_dev->data->name is to keep
> device name, all other PMDs use this way, not for Linux interface name.
> 
> Current tap PMD, first gives "dtap0" to rte_eth_dev_allocate() as
> argument, which sets device name to this value. Later with snprintf()
> overwrites the device name with correct value, I think better thing to
> do is give correct argument to rte_eth_dev_allocate() and remove snprintf()

All of these different names for the device is driving me crazy :-)

As long as the name of the device as seen from the ifconfig command or host facing name is dtapX then I am ok. We need to clean this up somehow as it is very confusing.

> 
>> 
>> Signed-off-by: Pascal Mazon <pascal.mazon@6wind.com>
>> ---
>> drivers/net/tap/rte_eth_tap.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>> 
>> diff --git a/drivers/net/tap/rte_eth_tap.c b/drivers/net/tap/rte_eth_tap.c
>> index 47a706070652..14c345f07afa 100644
>> --- a/drivers/net/tap/rte_eth_tap.c
>> +++ b/drivers/net/tap/rte_eth_tap.c
>> @@ -691,7 +691,7 @@ eth_dev_tap_create(const char *name, char *tap_name)
>> 	dev->driver = NULL;
>> 	dev->rx_pkt_burst = pmd_rx_burst;
>> 	dev->tx_pkt_burst = pmd_tx_burst;
>> -	snprintf(dev->data->name, sizeof(dev->data->name), "%s", name);
>> +	snprintf(dev->data->name, sizeof(dev->data->name), "%s", tap_name);
>> 
>> 	/* Presetup the fds to -1 as being not valid */
>> 	for (i = 0; i < RTE_PMD_TAP_MAX_QUEUES; i++) {
>> 
> 

Regards,
Keith
Ferruh Yigit March 6, 2017, 2:32 p.m. UTC | #4 
On 3/6/2017 2:15 PM, Wiles, Keith wrote:
> 
>> On Mar 5, 2017, at 3:35 PM, Yigit, Ferruh <ferruh.yigit@intel.com> wrote:
>>
>> On 3/3/2017 8:54 AM, Pascal Mazon wrote:
>>> The call to rte_eth_dev_allocate(tap_name) sets dev->data->name to
>>> tap_name (e.g. "dtap0").
>>>
>>> A look-up using tap_name is expected to return this device, not a
>>> look-up using name (e.g. "net_tap0").
>>
>> This will break rte_pmd_tap_remove(), because it gets device name
>> (net_tap0) as parameter.
>>
>> And logically this is wrong too, current eth_dev->data->name is to keep
>> device name, all other PMDs use this way, not for Linux interface name.
>>
>> Current tap PMD, first gives "dtap0" to rte_eth_dev_allocate() as
>> argument, which sets device name to this value. Later with snprintf()
>> overwrites the device name with correct value, I think better thing to
>> do is give correct argument to rte_eth_dev_allocate() and remove snprintf()
> 
> All of these different names for the device is driving me crazy :-)
> 
> As long as the name of the device as seen from the ifconfig command or host facing name is dtapX then I am ok. We need to clean this up somehow as it is very confusing.

It is not that bad indeed, from DPDK point of view, there is device and
driver name, and specific to tap, there is also Linux device (interface)
name.

Using Linux device name for DPDK device structures caused confusion for
tap, but I am not sure what to clean up.

> 
>>
>>>
>>> Signed-off-by: Pascal Mazon <pascal.mazon@6wind.com>
>>> ---
>>> drivers/net/tap/rte_eth_tap.c | 2 +-
>>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/net/tap/rte_eth_tap.c b/drivers/net/tap/rte_eth_tap.c
>>> index 47a706070652..14c345f07afa 100644
>>> --- a/drivers/net/tap/rte_eth_tap.c
>>> +++ b/drivers/net/tap/rte_eth_tap.c
>>> @@ -691,7 +691,7 @@ eth_dev_tap_create(const char *name, char *tap_name)
>>> 	dev->driver = NULL;
>>> 	dev->rx_pkt_burst = pmd_rx_burst;
>>> 	dev->tx_pkt_burst = pmd_tx_burst;
>>> -	snprintf(dev->data->name, sizeof(dev->data->name), "%s", name);
>>> +	snprintf(dev->data->name, sizeof(dev->data->name), "%s", tap_name);
>>>
>>> 	/* Presetup the fds to -1 as being not valid */
>>> 	for (i = 0; i < RTE_PMD_TAP_MAX_QUEUES; i++) {
>>>
>>
> 
> Regards,
> Keith
>
diff mbox

Patch

diff --git a/drivers/net/tap/rte_eth_tap.c b/drivers/net/tap/rte_eth_tap.c
index 47a706070652..14c345f07afa 100644
--- a/drivers/net/tap/rte_eth_tap.c
+++ b/drivers/net/tap/rte_eth_tap.c
@@ -691,7 +691,7 @@  eth_dev_tap_create(const char *name, char *tap_name)
 	dev->driver = NULL;
 	dev->rx_pkt_burst = pmd_rx_burst;
 	dev->tx_pkt_burst = pmd_tx_burst;
-	snprintf(dev->data->name, sizeof(dev->data->name), "%s", name);
+	snprintf(dev->data->name, sizeof(dev->data->name), "%s", tap_name);
 
 	/* Presetup the fds to -1 as being not valid */
 	for (i = 0; i < RTE_PMD_TAP_MAX_QUEUES; i++) {