Re: [dpdk-dev] [PATCH] kni: implement header_ops parse method

[Ferruh Yigit <ferruh.yigit@intel.com>]

On 1/24/2019 9:18 AM, Igor Ryzhov wrote:
> Hi Ferruh,
> 
> What about this patch?
> Can you merge it as-is, or should I change it to use relevant eth_header_ops
> functions? Or maybe completely use eth_header_ops?

Hi Igor,

I am not clear about motivation of the patch, what use case enabled by this
patch? What is not working with current code?
I am for rejecting the patch without need justified.

And if the need is justified, still there is a question that why not use
'eth_header_parse()' directly but implement our copy?


And an extended question/investigation about why not use 'eth_header_ops', which
seems done intentionally but I am missing the reasoning.

> 
> Best regards,
> Igor
> 
> On Fri, Nov 30, 2018 at 10:07 PM Igor Ryzhov <iryzhov@nfware.com
> <mailto:iryzhov@nfware.com>> wrote:
> 
>     Hi Ferruh,
> 
>     header_ops.parse method is used by raw-sockets to fill sockaddr_ll structure.
>     It is used, for example, in isisd for frrouting.
> 
>     Regarding your question about eth_header_ops – I, unfortunately, don't know
>     why .cache and .cache_update are disabled for KNI.
>     I also think that it will be better to use default eth_header_ops.
> 
>     Best regards,
>     Igor
> 
>     On Tue, Oct 2, 2018 at 7:58 PM Ferruh Yigit <ferruh.yigit@intel.com
>     <mailto:ferruh.yigit@intel.com>> wrote:
> 
>         On 9/27/2018 1:02 AM, Igor Ryzhov wrote:
>         > Signed-off-by: Igor Ryzhov <iryzhov@nfware.com
>         <mailto:iryzhov@nfware.com>>
> 
>         Hi Igor,
> 
>         What is the motivation to add this support? What is enabled by this?
> 
> 
>         Meanwhile, why we are not using eth_header_ops, which is already set by
>         ether_setup().
>         To disable .cache & .cache_update?
> 
>         If so why not using relevant eth_header_ops (eth_header,
>         eth_header_parse ..)
>         instead of implementing ours?
> 
>         > ---
>         >  kernel/linux/kni/kni_net.c | 14 ++++++++++++++
>         >  1 file changed, 14 insertions(+)
>         >
>         > diff --git a/kernel/linux/kni/kni_net.c b/kernel/linux/kni/kni_net.c
>         > index 7fcfa106c..128a5477c 100644
>         > --- a/kernel/linux/kni/kni_net.c
>         > +++ b/kernel/linux/kni/kni_net.c
>         > @@ -678,6 +678,19 @@ kni_net_header(struct sk_buff *skb, struct
>         net_device *dev,
>         >       return dev->hard_header_len;
>         >  }
>         > 
>         > +/*
>         > + *  Extract hardware address from packet
>         > + */
>         > +static int
>         > +kni_net_header_parse(const struct sk_buff *skb, unsigned char *haddr)
>         > +{
>         > +     const struct ethhdr *eth = eth_hdr(skb);
>         > +
>         > +     memcpy(haddr, eth->h_source, ETH_ALEN);
>         > +
>         > +     return ETH_ALEN;
>         > +}
>         > +
>         >  /*
>         >   * Re-fill the eth header
>         >   */
>         > @@ -739,6 +752,7 @@ kni_net_change_carrier(struct net_device *dev,
>         bool new_carrier)
>         > 
>         >  static const struct header_ops kni_net_header_ops = {
>         >       .create  = kni_net_header,
>         > +     .parse   = kni_net_header_parse,
>         >  #ifdef HAVE_REBUILD_HEADER
>         >       .rebuild = kni_net_rebuild_header,
>         >  #endif /* < 4.1.0  */
>         >
>