[PATCH] netifd: system-linux: add dev_type info for ubus network.device status

[Philip Prindeville]

This is probably after the fact, but...


> On Dec 6, 2021, at 3:15 AM, Florian Eckert <fe at dev.tdt.de> wrote:
> 
> Every network device has a type. This is stored in the sysfs under
> '/sys/class/net/<device>/type' as a number and can be queried.
> 
> This commit adds this information as a string to the ubus.
> 'ubus call network.device status'
> {
> ...
> 	"eth0": {
> 		"dev_type": "ETHER",
> ...
> }
> 
> Signed-off-by: Florian Eckert <fe at dev.tdt.de>
> ---
> system-linux.c | 21 +++++++++++++++++++++
> system.h       | 36 ++++++++++++++++++++++++++++++++++++
> 2 files changed, 57 insertions(+)
> 
> diff --git a/system-linux.c b/system-linux.c
> index e768853..4ac34cc 100644
> --- a/system-linux.c
> +++ b/system-linux.c
> @@ -2395,6 +2395,17 @@ system_if_force_external(const char *ifname)
> 	return stat(dev_sysfs_path(ifname, "phy80211"), &s) == 0;
> }
> 
> +static inline unsigned short netdev_type_pos(unsigned short dev_type)


Is there not a typedef'd definition for this?  If not, should we add one like "dev_type_t"?


> +{
> +	int i;


This can only be a positive (cardinal) number... therefore, please use an unsigned instead.


> +
> +	for (i = 0; i < ARRAY_SIZE(netdev_type_number); i++)
> +		if (netdev_type_number[i] == dev_type)
> +			return i;
> +	/* the last key is used by default */
> +	return ARRAY_SIZE(netdev_type_number) - 1;
> +}
> +
> int
> system_if_dump_info(struct device *dev, struct blob_buf *b)
> {
> @@ -2402,6 +2413,9 @@ system_if_dump_info(struct device *dev, struct blob_buf *b)
> 	struct ifreq ifr;
> 	char *s;
> 	void *c;
> +	unsigned short dev_type = 0;
> +	int i;


netdev_type_pos() returns an unsigned short.  Please make this concur.


> +	char buf[10];
> 
> 	memset(&ecmd, 0, sizeof(ecmd));
> 	memset(&ifr, 0, sizeof(ifr));
> @@ -2430,6 +2444,13 @@ system_if_dump_info(struct device *dev, struct blob_buf *b)
> 		blobmsg_add_u8(b, "autoneg", !!ecmd.autoneg);
> 	}
> 
> +	if (!system_get_dev_sysfs("type", dev->ifname, buf, sizeof(buf))) {
> +		dev_type = strtoul(buf, NULL, 0);
> +		i = netdev_type_pos(dev_type);
> +		blobmsg_add_string(b, "dev_type", netdev_type_name[i]);
> +		netifd_log_message(L_CRIT, "Interface type %s detected\n", netdev_type_name[i]);
> +	}
> +
> 	return 0;
> }
> 
> diff --git a/system.h b/system.h
> index 1f7037d..fdd8a64 100644
> --- a/system.h
> +++ b/system.h
> @@ -23,6 +23,42 @@
> #include "iprule.h"
> #include "utils.h"
> 
> +static const unsigned short netdev_type_number[] = {
> +	ARPHRD_NETROM, ARPHRD_ETHER, ARPHRD_EETHER, ARPHRD_AX25,
> +	ARPHRD_PRONET, ARPHRD_CHAOS, ARPHRD_IEEE802, ARPHRD_ARCNET,
> +	ARPHRD_APPLETLK, ARPHRD_DLCI, ARPHRD_ATM, ARPHRD_METRICOM,
> +	ARPHRD_IEEE1394, ARPHRD_EUI64, ARPHRD_INFINIBAND, ARPHRD_SLIP,
> +	ARPHRD_CSLIP, ARPHRD_SLIP6, ARPHRD_CSLIP6, ARPHRD_RSRVD,
> +	ARPHRD_ADAPT, ARPHRD_ROSE, ARPHRD_X25, ARPHRD_HWX25,
> +	ARPHRD_PPP, ARPHRD_CISCO, ARPHRD_LAPB, ARPHRD_DDCMP,
> +	ARPHRD_RAWHDLC, ARPHRD_TUNNEL, ARPHRD_TUNNEL6, ARPHRD_FRAD,
> +	ARPHRD_SKIP, ARPHRD_LOOPBACK, ARPHRD_LOCALTLK, ARPHRD_FDDI,
> +	ARPHRD_BIF, ARPHRD_SIT, ARPHRD_IPDDP, ARPHRD_IPGRE,
> +	ARPHRD_PIMREG, ARPHRD_HIPPI, ARPHRD_ASH, ARPHRD_ECONET,
> +	ARPHRD_IRDA, ARPHRD_FCPP, ARPHRD_FCAL, ARPHRD_FCPL,
> +	ARPHRD_FCFABRIC, ARPHRD_IEEE80211, ARPHRD_IEEE80211_PRISM,
> +	ARPHRD_IEEE80211_RADIOTAP, ARPHRD_PHONET, ARPHRD_PHONET_PIPE,
> +	ARPHRD_IEEE802154, ARPHRD_VOID, ARPHRD_NONE
> +};
> +
> +static const char *const netdev_type_name[] = {
> +	"NETROM", "ETHER", "EETHER", "AX25",
> +	"PRONET", "CHAOS", "IEEE802", "ARCNET",
> +	"APPLETLK", "DLCI", "ATM", "METRICOM",
> +	"IEEE1394", "EUI64", "INFINIBAND", "SLIP",
> +	"CSLIP", "SLIP6", "CSLIP6", "RSRVD",
> +	"ADAPT", "ROSE", "X25", "HWX25",
> +	"PPP", "CISCO", "LAPB", "DDCMP",
> +	"RAWHDLC", "TUNNEL", "TUNNEL6", "FRAD",
> +	"SKIP", "LOOPBACK", "LOCALTLK", "FDDI",
> +	"BIF", "SIT", "IPDDP", "IPGRE",
> +	"PIMREG", "HIPPI", "ASH", "ECONET",
> +	"IRDA", "FCPP", "FCAL", "FCPL",
> +	"FCFABRIC", "IEEE80211", "IEEE80211_PRISM",
> +	"IEEE80211_RADIOTAP", "PHONET", "PHONET_PIPE",
> +	"IEEE802154", "VOID", "NONE"
> +};


I'm worried about holes/discontinuities in ARPHRD_* space... would that break things?  I guess this could be a sparse array...

-Philip


> +
> enum tunnel_param {
> 	TUNNEL_ATTR_TYPE,
> 	TUNNEL_ATTR_REMOTE,
> -- 
> 2.20.1