proposal: x/sys/unix: Add support for ethtool_cmd and ethtool_value

[dkumazaw]

For invoking ioctl with the SIOCETHTOOL request, only EthtoolDrvinfo is supported by x/sys/unix at the moment. This is a proposal to implement support for ethtool_cmd and ethtool_value structs as follows:

1. Add new structs

   
   type EthtoolCmd struct {
   // struct ethtool_cmd from ethtool.h
   }

type EthtoolValue struct { // struct ethtool_value from ethtool.h }

2. Add new ioctl wrapper functions

func IoctlEthtoolCmd(fd int, ifname string, eCmd EthtoolCmd) error func IoctlEthtoolValue(fd int, ifname string, eValue EthtoolValue) error

[ianlancetaylor]

Should we pass an ioctl code to IoctlEthtoolCmd and IoctlEthtoolValue?

[ianlancetaylor]

In my copy of <linux/ethtool.h>, struct ethtool_cmd is marked as deprecated in favor of struct ethtool_link_settings.

I don't understand what the ifname parameter is for.

There seem to be a lot of backward compatibility concerns here; does this need to be in x/sys/unix, or would it be better in a third_party package?

[dkumazaw]

Thank you for taking a look at this proposal, @ianlancetaylor.

In my copy of <linux/ethtool.h>, struct ethtool_cmd is marked as deprecated in favor of struct ethtool_link_settings. I don't understand what the ifname parameter is for.

I made the following edits based on your comments:

1. Use the non-deprecated struct ethtool_link_settings instead of ethtool_cmd
2. Instead of accepting the struct pointer as an argument, build a "Get" function that corresponds to the struct, and let the function return the desired struct. This follows the pattern established by IoctlGetEthtoolDrvinfo and should clearly signal that we're getting the struct for the device specified by ifname.
3. For ethtool_value, since various commands produce this struct as per linux/ethtool.h, expose one "Get" function per command (although I can also see this parameterized as an argument).

type EthtoolLinkSettings struct {
  // struct ethtool_link_settings from linux/ethtool.h ...
}

// IoctlGetEthtoolLinkSettings fetches link settings for the network device specified by ifname
func IoctlGetEthtoolLinkSettings(fd int, ifname string) (*EthtoolLinkSettings, error) // Uses .Cmd=ETHTOOL_GLINKSETTINGS

type EthtoolValue struct {
  // struct ethtool_value from linux/ethtool.h ...
}

// IoctlGetEthtoolLinkStatus fetches link status for the network device specified by ifname
func IoctlGetLinkStatus(fd int, ifname string) (*EthtoolValue, error) // Uses .Cmd=ETHTOOL_GLINK

// Get functions for other commands that return *EthtoolValue can be implemented with a similar signature

There seem to be a lot of backward compatibility concerns here; does this need to be in x/sys/unix, or would it be better in a third_party package?

Do the above edits clear the concerns around backwards compatibility?

[ianlancetaylor]

CC @mdlayher

[mdlayher]

Seems reasonable to me, although I should note there has been an ongoing effort to expose the ethtool interface via genetlink. I have a library at https://github.com/mdlayher/ethtool that is far from complete but may be more easily extended than dealing with the ioctls.

https://www.kernel.org/doc/html/latest/networking/ethtool-netlink.html

But since they're part of the Linux UAPI I also have no objections adding more ioctl calls to x/sys/unix.

[dkumazaw]

Great, thank you for the feedback. I'll proceed to implementation unless there are further concerns.