| Message ID | 191C4D2B-D3AE-4E73-82FA-9BA76ABC9303@vmware.com |
|---|---|
| State | Not Applicable |
| Headers | show |
Hi Darrell, Good catch. Thanks. One comment inline. Best regards, Ilya Maximets. On 16.05.2017 20:14, Darrell Ball wrote: > I applied the following incremental to fix a missing free on error, but otherwise is fine > Can you fold in the incremental. > > > diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c > index 56b9d25..25d9c9b 100644 > --- a/lib/netdev-dpdk.c > +++ b/lib/netdev-dpdk.c > @@ -1128,7 +1128,7 @@ netdev_dpdk_process_devargs(const char *devargs, char **errp) > } else { > /* Attach unsuccessful */ > VLOG_WARN_BUF(errp, "Error attaching device '%s' to DPDK", devargs); > - return -1; > + new_port_id = UINT8_MAX; 'rte_eth_dev_get_port_by_name' and 'rte_eth_dev_attach' set a valid 'new_port_id' only on success. So, we don't need to reinitialize it here. Just removing of return should be enough. > } > } > > > On 5/12/17, 8:10 AM, "Ilya Maximets" <i.maximets@samsung.com> wrote: > > 'devargs' for virtual devices contains not only name but > also a list of arguments like this: > > 'net_pcap0,rx_pcap=file_rx.pcap,tx_pcap=file_tx.pcap' > or > 'eth_af_packet0,iface=eth0' > > We must cut off the arguments from this string before calling > 'rte_eth_dev_get_port_by_name()' to avoid double attaching of > the same device. > > CC: Ciara Loftus <ciara.loftus@intel.com> > Fixes: 69876ed78611 ("netdev-dpdk: Add support for virtual DPDK PMDs (vdevs)") > Signed-off-by: Ilya Maximets <i.maximets@samsung.com> > --- > > lib/netdev-dpdk.c | 5 ++++- > 1 file changed, 4 insertions(+), 1 deletion(-) > > diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c > index 609b8da..56b9d25 100644 > --- a/lib/netdev-dpdk.c > +++ b/lib/netdev-dpdk.c > @@ -1114,10 +1114,12 @@ netdev_dpdk_lookup_by_port_id(int port_id) > static int > netdev_dpdk_process_devargs(const char *devargs, char **errp) > { > + /* Get the name up to the first comma. */ > + char *name = xmemdup0(devargs, strcspn(devargs, ",")); > uint8_t new_port_id = UINT8_MAX; > > if (!rte_eth_dev_count() > - || rte_eth_dev_get_port_by_name(devargs, &new_port_id) > + || rte_eth_dev_get_port_by_name(name, &new_port_id) > || !rte_eth_dev_is_valid_port(new_port_id)) { > /* Device not found in DPDK, attempt to attach it */ > if (!rte_eth_dev_attach(devargs, &new_port_id)) { > @@ -1130,6 +1132,7 @@ netdev_dpdk_process_devargs(const char *devargs, char **errp) > } > } > > + free(name); > return new_port_id; > } > > -- > 2.7.4 > > >
On 17.05.2017 08:53, Ilya Maximets wrote: > Hi Darrell, > > Good catch. Thanks. One comment inline. > > Best regards, Ilya Maximets. > > On 16.05.2017 20:14, Darrell Ball wrote: >> I applied the following incremental to fix a missing free on error, but otherwise is fine >> Can you fold in the incremental. >> >> >> diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c >> index 56b9d25..25d9c9b 100644 >> --- a/lib/netdev-dpdk.c >> +++ b/lib/netdev-dpdk.c >> @@ -1128,7 +1128,7 @@ netdev_dpdk_process_devargs(const char *devargs, char **errp) >> } else { >> /* Attach unsuccessful */ >> VLOG_WARN_BUF(errp, "Error attaching device '%s' to DPDK", devargs); >> - return -1; >> + new_port_id = UINT8_MAX; > > 'rte_eth_dev_get_port_by_name' and 'rte_eth_dev_attach' set a valid > 'new_port_id' only on success. So, we don't need to reinitialize it > here. Just removing of return should be enough. I just found that this change also was mistakenly moved to the third patch. Oh.. Kind of bad luck with splitting of this patches. Also, I think, I'll keep this reinitialization just to make code more clear. >> } >> } >> >> >> On 5/12/17, 8:10 AM, "Ilya Maximets" <i.maximets@samsung.com> wrote: >> >> 'devargs' for virtual devices contains not only name but >> also a list of arguments like this: >> >> 'net_pcap0,rx_pcap=file_rx.pcap,tx_pcap=file_tx.pcap' >> or >> 'eth_af_packet0,iface=eth0' >> >> We must cut off the arguments from this string before calling >> 'rte_eth_dev_get_port_by_name()' to avoid double attaching of >> the same device. >> >> CC: Ciara Loftus <ciara.loftus@intel.com> >> Fixes: 69876ed78611 ("netdev-dpdk: Add support for virtual DPDK PMDs (vdevs)") >> Signed-off-by: Ilya Maximets <i.maximets@samsung.com> >> --- >> >> lib/netdev-dpdk.c | 5 ++++- >> 1 file changed, 4 insertions(+), 1 deletion(-) >> >> diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c >> index 609b8da..56b9d25 100644 >> --- a/lib/netdev-dpdk.c >> +++ b/lib/netdev-dpdk.c >> @@ -1114,10 +1114,12 @@ netdev_dpdk_lookup_by_port_id(int port_id) >> static int >> netdev_dpdk_process_devargs(const char *devargs, char **errp) >> { >> + /* Get the name up to the first comma. */ >> + char *name = xmemdup0(devargs, strcspn(devargs, ",")); >> uint8_t new_port_id = UINT8_MAX; >> >> if (!rte_eth_dev_count() >> - || rte_eth_dev_get_port_by_name(devargs, &new_port_id) >> + || rte_eth_dev_get_port_by_name(name, &new_port_id) >> || !rte_eth_dev_is_valid_port(new_port_id)) { >> /* Device not found in DPDK, attempt to attach it */ >> if (!rte_eth_dev_attach(devargs, &new_port_id)) { >> @@ -1130,6 +1132,7 @@ netdev_dpdk_process_devargs(const char *devargs, char **errp) >> } >> } >> >> + free(name); >> return new_port_id; >> } >> >> -- >> 2.7.4 >> >> >>
On 5/17/17, 4:16 AM, "Ilya Maximets" <i.maximets@samsung.com> wrote:
On 17.05.2017 08:53, Ilya Maximets wrote:
> Hi Darrell,
>
> Good catch. Thanks. One comment inline.
>
> Best regards, Ilya Maximets.
>
> On 16.05.2017 20:14, Darrell Ball wrote:
>> I applied the following incremental to fix a missing free on error, but otherwise is fine
>> Can you fold in the incremental.
>>
>>
>> diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c
>> index 56b9d25..25d9c9b 100644
>> --- a/lib/netdev-dpdk.c
>> +++ b/lib/netdev-dpdk.c
>> @@ -1128,7 +1128,7 @@ netdev_dpdk_process_devargs(const char *devargs, char **errp)
>> } else {
>> /* Attach unsuccessful */
>> VLOG_WARN_BUF(errp, "Error attaching device '%s' to DPDK", devargs);
>> - return -1;
>> + new_port_id = UINT8_MAX;
>
> 'rte_eth_dev_get_port_by_name' and 'rte_eth_dev_attach' set a valid
> 'new_port_id' only on success. So, we don't need to reinitialize it
> here. Just removing of return should be enough.
I just found that this change also was mistakenly moved to the third patch.
Oh.. Kind of bad luck with splitting of this patches.
Also, I think, I'll keep this reinitialization just to make code more clear.
In your previous response, you were arguing that removing the return was good
enough. Now, you are saying the re-initialization is ok since you had something
similar in your 3rd patch – hmm, fine.
>> }
>> }
>>
>>
>> On 5/12/17, 8:10 AM, "Ilya Maximets" <i.maximets@samsung.com> wrote:
>>
>> 'devargs' for virtual devices contains not only name but
>> also a list of arguments like this:
>>
>> 'net_pcap0,rx_pcap=file_rx.pcap,tx_pcap=file_tx.pcap'
>> or
>> 'eth_af_packet0,iface=eth0'
>>
>> We must cut off the arguments from this string before calling
>> 'rte_eth_dev_get_port_by_name()' to avoid double attaching of
>> the same device.
>>
>> CC: Ciara Loftus <ciara.loftus@intel.com>
>> Fixes: 69876ed78611 ("netdev-dpdk: Add support for virtual DPDK PMDs (vdevs)")
>> Signed-off-by: Ilya Maximets <i.maximets@samsung.com>
>> ---
>>
>> lib/netdev-dpdk.c | 5 ++++-
>> 1 file changed, 4 insertions(+), 1 deletion(-)
>>
>> diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c
>> index 609b8da..56b9d25 100644
>> --- a/lib/netdev-dpdk.c
>> +++ b/lib/netdev-dpdk.c
>> @@ -1114,10 +1114,12 @@ netdev_dpdk_lookup_by_port_id(int port_id)
>> static int
>> netdev_dpdk_process_devargs(const char *devargs, char **errp)
>> {
>> + /* Get the name up to the first comma. */
>> + char *name = xmemdup0(devargs, strcspn(devargs, ","));
>> uint8_t new_port_id = UINT8_MAX;
>>
>> if (!rte_eth_dev_count()
>> - || rte_eth_dev_get_port_by_name(devargs, &new_port_id)
>> + || rte_eth_dev_get_port_by_name(name, &new_port_id)
>> || !rte_eth_dev_is_valid_port(new_port_id)) {
>> /* Device not found in DPDK, attempt to attach it */
>> if (!rte_eth_dev_attach(devargs, &new_port_id)) {
>> @@ -1130,6 +1132,7 @@ netdev_dpdk_process_devargs(const char *devargs, char **errp)
>> }
>> }
>>
>> + free(name);
>> return new_port_id;
>> }
>>
>> --
>> 2.7.4
>>
>>
>>
On 17.05.2017 18:01, Darrell Ball wrote: > > > On 5/17/17, 4:16 AM, "Ilya Maximets" <i.maximets@samsung.com> wrote: > > On 17.05.2017 08:53, Ilya Maximets wrote: > > Hi Darrell, > > > > Good catch. Thanks. One comment inline. > > > > Best regards, Ilya Maximets. > > > > On 16.05.2017 20:14, Darrell Ball wrote: > >> I applied the following incremental to fix a missing free on error, but otherwise is fine > >> Can you fold in the incremental. > >> > >> > >> diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c > >> index 56b9d25..25d9c9b 100644 > >> --- a/lib/netdev-dpdk.c > >> +++ b/lib/netdev-dpdk.c > >> @@ -1128,7 +1128,7 @@ netdev_dpdk_process_devargs(const char *devargs, char **errp) > >> } else { > >> /* Attach unsuccessful */ > >> VLOG_WARN_BUF(errp, "Error attaching device '%s' to DPDK", devargs); > >> - return -1; > >> + new_port_id = UINT8_MAX; > > > > 'rte_eth_dev_get_port_by_name' and 'rte_eth_dev_attach' set a valid > > 'new_port_id' only on success. So, we don't need to reinitialize it > > here. Just removing of return should be enough. > > I just found that this change also was mistakenly moved to the third patch. > Oh.. Kind of bad luck with splitting of this patches. > Also, I think, I'll keep this reinitialization just to make code more clear. > > In your previous response, you were arguing that removing the return was good > enough. Now, you are saying the re-initialization is ok since you had something > similar in your 3rd patch – hmm, fine. There is one more reason. 'rte_eth_dev_get_port_by_name' actually changes the new_port_id even on failure. It sets new_port_id to RTE_MAX_ETHPORTS. It's harmless, but looks logically wrong. This is actually undocumented behaviour and I decided to re-initialize just to avoid any possible issue in the future. In general, I think, that 'rte_eth_dev_get_port_by_name' shouldn't touch the new_port_id in case of failure. I'll send corresponding patch to DPDK. > >> } > >> } > >> > >> > >> On 5/12/17, 8:10 AM, "Ilya Maximets" <i.maximets@samsung.com> wrote: > >> > >> 'devargs' for virtual devices contains not only name but > >> also a list of arguments like this: > >> > >> 'net_pcap0,rx_pcap=file_rx.pcap,tx_pcap=file_tx.pcap' > >> or > >> 'eth_af_packet0,iface=eth0' > >> > >> We must cut off the arguments from this string before calling > >> 'rte_eth_dev_get_port_by_name()' to avoid double attaching of > >> the same device. > >> > >> CC: Ciara Loftus <ciara.loftus@intel.com> > >> Fixes: 69876ed78611 ("netdev-dpdk: Add support for virtual DPDK PMDs (vdevs)") > >> Signed-off-by: Ilya Maximets <i.maximets@samsung.com> > >> --- > >> > >> lib/netdev-dpdk.c | 5 ++++- > >> 1 file changed, 4 insertions(+), 1 deletion(-) > >> > >> diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c > >> index 609b8da..56b9d25 100644 > >> --- a/lib/netdev-dpdk.c > >> +++ b/lib/netdev-dpdk.c > >> @@ -1114,10 +1114,12 @@ netdev_dpdk_lookup_by_port_id(int port_id) > >> static int > >> netdev_dpdk_process_devargs(const char *devargs, char **errp) > >> { > >> + /* Get the name up to the first comma. */ > >> + char *name = xmemdup0(devargs, strcspn(devargs, ",")); > >> uint8_t new_port_id = UINT8_MAX; > >> > >> if (!rte_eth_dev_count() > >> - || rte_eth_dev_get_port_by_name(devargs, &new_port_id) > >> + || rte_eth_dev_get_port_by_name(name, &new_port_id) > >> || !rte_eth_dev_is_valid_port(new_port_id)) { > >> /* Device not found in DPDK, attempt to attach it */ > >> if (!rte_eth_dev_attach(devargs, &new_port_id)) { > >> @@ -1130,6 +1132,7 @@ netdev_dpdk_process_devargs(const char *devargs, char **errp) > >> } > >> } > >> > >> + free(name); > >> return new_port_id; > >> } > >> > >> -- > >> 2.7.4 > >> > >> > >> > >
diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c index 56b9d25..25d9c9b 100644 --- a/lib/netdev-dpdk.c +++ b/lib/netdev-dpdk.c @@ -1128,7 +1128,7 @@ netdev_dpdk_process_devargs(const char *devargs, char **errp) } else { /* Attach unsuccessful */ VLOG_WARN_BUF(errp, "Error attaching device '%s' to DPDK", devargs); - return -1; + new_port_id = UINT8_MAX; } }
I applied the following incremental to fix a missing free on error, but otherwise is fine Can you fold in the incremental. On 5/12/17, 8:10 AM, "Ilya Maximets" <i.maximets@samsung.com> wrote: 'devargs' for virtual devices contains not only name but also a list of arguments like this: 'net_pcap0,rx_pcap=file_rx.pcap,tx_pcap=file_tx.pcap' or 'eth_af_packet0,iface=eth0' We must cut off the arguments from this string before calling 'rte_eth_dev_get_port_by_name()' to avoid double attaching of the same device. CC: Ciara Loftus <ciara.loftus@intel.com> Fixes: 69876ed78611 ("netdev-dpdk: Add support for virtual DPDK PMDs (vdevs)") Signed-off-by: Ilya Maximets <i.maximets@samsung.com> --- lib/netdev-dpdk.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c index 609b8da..56b9d25 100644 --- a/lib/netdev-dpdk.c +++ b/lib/netdev-dpdk.c @@ -1114,10 +1114,12 @@ netdev_dpdk_lookup_by_port_id(int port_id) static int netdev_dpdk_process_devargs(const char *devargs, char **errp) { + /* Get the name up to the first comma. */ + char *name = xmemdup0(devargs, strcspn(devargs, ",")); uint8_t new_port_id = UINT8_MAX; if (!rte_eth_dev_count() - || rte_eth_dev_get_port_by_name(devargs, &new_port_id) + || rte_eth_dev_get_port_by_name(name, &new_port_id) || !rte_eth_dev_is_valid_port(new_port_id)) { /* Device not found in DPDK, attempt to attach it */ if (!rte_eth_dev_attach(devargs, &new_port_id)) { @@ -1130,6 +1132,7 @@ netdev_dpdk_process_devargs(const char *devargs, char **errp) } } + free(name); return new_port_id; } -- 2.7.4