Re: [PATCH net-next v6 08/15] ethtool: move string arrays into common file

[Michal Kubecek <mkubecek@suse.cz>]

On Wed, Jul 03, 2019 at 03:44:52PM +0200, Jiri Pirko wrote:
> Tue, Jul 02, 2019 at 01:50:19PM CEST, mkubecek@suse.cz wrote:
> >Introduce file net/ethtool/common.c for code shared by ioctl and netlink
> >ethtool interface. Move name tables of features, RSS hash functions,
> >tunables and PHY tunables into this file.
> >
> >Signed-off-by: Michal Kubecek <mkubecek@suse.cz>
> >---
> > net/ethtool/Makefile |  2 +-
> > net/ethtool/common.c | 84 ++++++++++++++++++++++++++++++++++++++++++++
> > net/ethtool/common.h | 17 +++++++++
> > net/ethtool/ioctl.c  | 83 ++-----------------------------------------
> > 4 files changed, 104 insertions(+), 82 deletions(-)
> > create mode 100644 net/ethtool/common.c
> > create mode 100644 net/ethtool/common.h
> >
> >diff --git a/net/ethtool/Makefile b/net/ethtool/Makefile
> >index 482fdb9380fa..11782306593b 100644
> >--- a/net/ethtool/Makefile
> >+++ b/net/ethtool/Makefile
> >@@ -1,6 +1,6 @@
> > # SPDX-License-Identifier: GPL-2.0
> > 
> >-obj-y				+= ioctl.o
> >+obj-y				+= ioctl.o common.o
> > 
> > obj-$(CONFIG_ETHTOOL_NETLINK)	+= ethtool_nl.o
> > 
> >diff --git a/net/ethtool/common.c b/net/ethtool/common.c
> >new file mode 100644
> >index 000000000000..b0ce420e994e
> >--- /dev/null
> >+++ b/net/ethtool/common.c
> >@@ -0,0 +1,84 @@
> >+// SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note
> >+
> >+#include "common.h"
> >+
> >+const char netdev_features_strings[NETDEV_FEATURE_COUNT][ETH_GSTRING_LEN] = {
> 
> const char *netdev_features_strings[NETDEV_FEATURE_COUNT] = {
> ?
> 
> Same with the other arrays.

These are not new tables, this patch only moves existing tables from
ioctl.c (originally net/core/ethtool.c) into common.c so that they can
be used by both ioctl and netlink code.

This fixed size string array format is used by ETHTOOL_GSTRINGS ioctl
command. So if we switch these into simple const char *table[], we can
get rid of some complexity in strset.c and bitset.c (the "simple" vs.
"legacy" string set mess) but we would have to convert them into the
fixed size string array in ioctl ETHTOOL_GSTRINGS handler. And then we
would also have to convert (or rather "index") string sets retrieved
from NIC driver (e.g. private flags, stats, tests) - which also means an
extra kmalloc() (or rather kmalloc_array()).

It an option I'm certainly open to if we agree on it but it's not for
free.

Michal