Re: [dpdk-dev] [PATCH 2/2] ethtool: add new library to provide ethtool-alike APIs

[Thomas Monjalon <thomas.monjalon@6wind.com>]

Wang, hope it's clear that any new development is welcomed.
One step before integration is to clearly explain why your code
is needed. That's why a nack vote may help to discuss and decide.

Comments below

2015-06-02 13:15, Wang, Liang-min:
> >2015-05-29 15:26, Liang-Min Larry Wang:
> >> adding a new library based upon ethdev APIs to provide API's that bear 
> >> the same functionality as ethtool_ops (linux/ethtool.h) and 
> >> net_device_ops (linux/netdevice.h).
> >> 
> >> Signed-off-by: Liang-Min Larry Wang <liang-min.wang@intel.com>
> >> ---
> >>  MAINTAINERS                                |   4 +
> >>  config/common_linuxapp                     |   5 +
> >>  lib/Makefile                               |   1 +
> >>  lib/librte_ethtool/Makefile                |  56 +++++++
> >>  lib/librte_ethtool/rte_ethtool.c           | 155 +++++++++++++++++
> >>  lib/librte_ethtool/rte_ethtool.h           | 257 +++++++++++++++++++++++++++++
> >>  lib/librte_ethtool/rte_ethtool_version.map |  18 ++
> >>  mk/rte.app.mk                              |   1 +
> >
> >NACK for several reasons:
> >- It's unclear what benefits this ethdev wrapper is bringing
> 
> Since ethtool is provided to assist users migrating from kernel ethtool/net_device_op based design to user-space DPDK device management. The ethtool API's are created to closely maintain its original interface, therefore this library depends on <linux/ethool.h>. To avoid pollute the existing ethdev interface, a new library is created. To minimize code replication and maintain closely 1:1 API definition with kernel space API, this interface is designed based upon available ethdev APIs and add additional dev_ops if it's necessary.
> 
> >- There is no obvious interest (how is it supposed to be used?)
> There are already two acknowledge on this release. Earlier comment on this patch has that " ... The API's for ethtool like things are valuable ..."

Stephen had some doubts about the real need and 2 people from Cisco
(who never contributed before) give their ack without justification.
Saying it's "valuable" or "very useful" is not enough.
A new library needs to demonstrate in which scenario the added-value is.
Sorry but you have to prove that it deserves to be maintained inside
the dpdk project.

> >- There is no update in the doc/ directory
> Need more guidance on that.

You probably have to add a new chapter in the programmer's guide.

> >Other comments:
> >- the patches are not versioned
> 
> There is version file. Not sure what do you mean "the patches are not versioned"

I mean there is no v2/v3 in the Subject. Please read
	http://dpdk.org/dev#send

> >- the copyright starts in 2010
> 
> Will fix that.
> 
> >I'm curious to understand how renaming rte_eth_dev_set_mtu() to
> >rte_ethtool_net_change_mtu() will help anyone.
> 
> As described, this interface is designed to provide API closely to kernel space ethtool ops and net_device_op.

But the application still needs to adapt the code to call rte_* functions.
So changing to rte_ethtool_net_change_mtu is equivalent to change to
the existing rte_eth_dev_set_mtu. I don't see the benefit.