[ovs-dev] [PATCH v2 1/3] netdev-dpdk: Fix double attaching of virtual devices.

[Darrell Ball]

On 5/17/17, 4:16 AM, "Ilya Maximets" <i.maximets at 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 at 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 at intel.com>
    >>     Fixes: 69876ed78611 ("netdev-dpdk: Add support for virtual DPDK PMDs (vdevs)")
    >>     Signed-off-by: Ilya Maximets <i.maximets at 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
    >>     
    >>     
    >>