Re: [PATCH v6] net/pcap: physical interface MAC address support

["Kuusisaari, Juhamatti (Coriant - FI/Espoo)" <juhamatti.kuusisaari@coriant.com>]

Hello Ferruh,

> On 9/10/2018 5:55 PM, Juhamatti Kuusisaari wrote:
> > At the moment, PCAP interfaces use dummy MAC by default. This change
> > adds support for selecting PCAP physical interface MAC with phy_mac=1
> > devarg. This allows to setup packet flows using the physical interface
> > MAC.
> >
> > Signed-off-by: Juhamatti Kuusisaari <juhamatti.kuusisaari@coriant.com>
> >
> > ---
> >  v6:
> >   * Review changes:
> >     * Clarified devarg applicability (applies to iface-only)
> >     * Introduced detailed documentation for the devarg
> >     * More checkpatches-fixes
> >  v5-v3:
> >   * FreeBSD related compilation and checkpatches-fixes
> >  v2:
> >   * FreeBSD support introduced
> >   * Release notes added
> >  v1:
> >   * phy_mac=1 devarg support
> 
> <...>
> 
> > +#elif defined(__FreeBSD__)
> 
> Just to double check did you check/verify below code on FreeBSD?

I did run a manual test of the added code and seems to work. I think it is still better to run DPDK on it to make sure everything works, so I'll do that before sending next revision.

> 
> <...>
> 
> > @@ -955,6 +1034,10 @@ eth_from_pcaps_common(struct
> rte_vdev_device *vdev,
> >  	else
> >  		(*internals)->if_index = if_nametoindex(pair->value);
> >
> > +	if (phy_mac && pair) /* phy_mac arg is applied only to iface */
> 
> Having this comment is good, but "iface" is so generic, it may be confusing for
> beyond this context, what about "only if iface devarg provided" kind of detail?

Yes, let's elaborate that.

> <...>
> 
> > @@ -989,6 +1073,19 @@ eth_from_pcaps(struct rte_vdev_device *vdev,
> >  	return 0;
> >  }
> >
> > +static int
> > +select_phy_mac(const char *key, const char *value, void *extra_args)
> > +{
> > +	if (extra_args) {
> > +		const int phy_mac = atoi(value);
> > +		int *enable_phy_mac = extra_args;
> > +
> > +		if (phy_mac)
> > +			*enable_phy_mac = 1;
> > +	}
> > +	return 0;
> > +}
> 
> This is causing build error because of "key" not used, needs __rte_unused
> marker.

Yes, need to fix this.

> <...>
> 
> > @@ -1031,6 +1129,16 @@ pmd_pcap_probe(struct rte_vdev_device *dev)
> >  	 * reading / writing
> >  	 */
> >  	if (rte_kvargs_count(kvlist, ETH_PCAP_IFACE_ARG) == 1) {
> > +		/*
> > +		 * We check first whether we want to use phy MAC of the
> PCAP
> > +		 * interface.
> > +		 */
> > +		if (rte_kvargs_count(kvlist, ETH_PCAP_PHY_MAC_ARG)) {
> 
> Do you need count check at all?

I think it is needed, as the arg may not exist there. At least to me calling process directly does not feel right, even if it would work.

> > +			ret = rte_kvargs_process(kvlist,
> ETH_PCAP_PHY_MAC_ARG,
> > +					&select_phy_mac, &phy_mac);
> > +			if (ret < 0)
> > +				goto free_kvlist;
> > +		}
> 
> I would prefer to see this block below ETH_PCAP_IFACE_ARG check, this
> block is
> for "iface", so it makes more sense to me first verify it, later verify phy_mac

Sure, I'll add it.

As there is already pcap-related MAC address patch merged, I'll need to do a rebase and recheck this patch functionality on top of that. It will take some time (which is a scarce resource at the moment), so I'll try to get this in again after 18.11.

Thanks,
--
 Juhamatti