Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
dkumazaw opened this issue Sep 18, 2023 · 6 comments
Open

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

dkumazaw opened this issue Sep 18, 2023 · 6 comments
Labels
Milestone

Comments

@dkumazaw
Copy link

dkumazaw commented Sep 18, 2023

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
}
  1. Add new ioctl wrapper functions
func IoctlEthtoolCmd(fd int, ifname string, eCmd *EthtoolCmd) error
func IoctlEthtoolValue(fd int, ifname string, eValue *EthtoolValue) error
@gopherbot gopherbot added this to the Proposal milestone Sep 18, 2023
@dkumazaw dkumazaw changed the title proposal: sys/unix: Add support for ethtool_cmd and ethtool_value proposal: x/sys/unix: Add support for ethtool_cmd and ethtool_value Sep 18, 2023
@ianlancetaylor
Copy link
Contributor

Should we pass an ioctl code to IoctlEthtoolCmd and IoctlEthtoolValue?

@ianlancetaylor
Copy link
Contributor

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
Copy link
Author

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
Copy link
Contributor

CC @mdlayher

@mdlayher
Copy link
Member

mdlayher commented Sep 18, 2023

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
Copy link
Author

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Status: Incoming
Development

No branches or pull requests

4 participants