Hi Ferruh, Can we proceed with this patch?
Igor On Thu, Jan 24, 2019 at 9:05 PM Igor Ryzhov <iryz...@nfware.com> wrote: > Hi Ferruh, > > Ok, no problem. Generally, it is needed for all applications using > packet(7) interface, running over KNI interfaces. > More specifically, one example of such application is FRRouting, I suppose > you are familiar with it. > FRR's ISIS daemon is using AF_PACKET sockets and checking received > sockaddr_ll. > Here is the link on one of the usages – > https://github.com/FRRouting/frr/blob/master/isisd/isis_pfpacket.c#L294 > > When we are good with motivation, I'll send v2 using eth_header_parse. > > Best regards, > Igor > > On Thu, Jan 24, 2019 at 8:15 PM Ferruh Yigit <ferruh.yi...@intel.com> > wrote: > >> On 1/24/2019 4:35 PM, Igor Ryzhov wrote: >> > Hi Ferruh, >> > >> > I already answered your question in my previous email, header_ops.parse >> method >> > is used by packet(7) interface for packet parsing and filling >> sockaddr_ll structure. >> > Here is the link on the usage – >> > >> https://elixir.bootlin.com/linux/latest/source/net/packet/af_packet.c#L2100 >> >> I saw your previous reply. That is why changed the question slightly, >> from 'what >> it does' to 'what is the use case'. >> Trying to understand do we need it, please help me understand. >> >> > >> > Regarding the question about eth_header_ops usage: >> > Right now both already existing functions, kni_net_header and >> > kni_net_rebuild_header, are implemented as copies, not using >> eth_header_ops >> > functions. >> >> OK, I see your reasoning, but if there is an already Linux API that does >> what we >> want, I suggest calling it instead of re-implementing it, unless there is >> a good >> reason to not do so. >> >> > That's why I think my patch should be accepted as is, and the problem of >> > eth_header_ops usage should be investigated separately, and possibly >> resolved by >> > a separate patch. >> >> Agreed, eth_header_ops usage should be investigated separately. >> >> > >> > Best regards, >> > Igor >> > >> > On Thu, Jan 24, 2019 at 5:10 PM Ferruh Yigit <ferruh.yi...@intel.com >> > <mailto:ferruh.yi...@intel.com>> wrote: >> > >> > 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 <iryz...@nfware.com >> > <mailto:iryz...@nfware.com> >> > > <mailto:iryz...@nfware.com <mailto:iryz...@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.yi...@intel.com >> > <mailto:ferruh.yi...@intel.com> >> > > <mailto:ferruh.yi...@intel.com <mailto:ferruh.yi...@intel.com>>> >> wrote: >> > > >> > > On 9/27/2018 1:02 AM, Igor Ryzhov wrote: >> > > > Signed-off-by: Igor Ryzhov <iryz...@nfware.com >> > <mailto:iryz...@nfware.com> >> > > <mailto:iryz...@nfware.com <mailto:iryz...@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 */ >> > > > >> > > >> > >> >>
