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

x/sys/unix: add IoctlSetIntPtr #46060

Open
lhl2617 opened this issue May 8, 2021 · 9 comments
Open

x/sys/unix: add IoctlSetIntPtr #46060

lhl2617 opened this issue May 8, 2021 · 9 comments

Comments

@lhl2617
Copy link

@lhl2617 lhl2617 commented May 8, 2021

What did you expect to see?

Original discussion: #46058

Some ioctl calls, such as https://www.kernel.org/doc/html/latest/watchdog/watchdog-api.html#setting-and-getting-the-timeout, modify the value in the ioctl call after the value is passed into the operation. Such ioctl calls are of the IOWR (IO write read) variant, as opposed to the IOR and IOW variants.

IoctlSetPointerInt is not able to return the modified value.

Proposed fix:

// IoctlSetModifiablePointerInt performs an ioctl operation which sets a
// pointer to an integer value on fd, using the specified request number.
//
// Use this if the ioctl operation may modify the value.
func IoctlSetModifiablePointerInt(fd int, req uint, value *int) error {
	return ioctl(fd, req, uintptr(unsafe.Pointer(value)))
}
@gopherbot gopherbot added this to the Unreleased milestone May 8, 2021
@lhl2617 lhl2617 changed the title x/sys/linux: Introduce ioctl function that return the modified pointer value x/sys/linux: add IoctlSetModifiablePointerInt that reflects changes in the ioctl value May 8, 2021
@gopherbot
Copy link

@gopherbot gopherbot commented May 8, 2021

Change https://golang.org/cl/318210 mentions this issue: x/sys/linux: add IoctlSetModifiablePointerInt that reflects changes in the ioctl value

@lhl2617
Copy link
Author

@lhl2617 lhl2617 commented May 9, 2021

Following the discussing in #46058, I further investigated: ioctl calls of the IOWR variant write the data to the kernel then read from it (see https://www.kernel.org/doc/html/latest/driver-api/ioctl.html). As of kernel v5.12 there are 200+ references to IOWR.

Previously I've been using https://github.com/vtolstov/go-ioctl which allows manual specification of ioctl calls, but having these automatically generated in x/sys would be neater and nicer.

@lhl2617 lhl2617 changed the title x/sys/linux: add IoctlSetModifiablePointerInt that reflects changes in the ioctl value proposal: x/sys/linux: add IoctlSetModifiablePointerInt that reflects changes in the ioctl value May 9, 2021
@lhl2617 lhl2617 changed the title proposal: x/sys/linux: add IoctlSetModifiablePointerInt that reflects changes in the ioctl value proposal: x/sys/unix/linux: add IoctlSetModifiablePointerInt that reflects changes in the ioctl value May 9, 2021
@lhl2617 lhl2617 changed the title proposal: x/sys/unix/linux: add IoctlSetModifiablePointerInt that reflects changes in the ioctl value proposal: x/sys/unix: add IoctlSetModifiablePointerInt that reflects changes in the ioctl value May 9, 2021
@ianlancetaylor ianlancetaylor added this to Incoming in Proposals May 11, 2021
@rsc
Copy link
Contributor

@rsc rsc commented May 12, 2021

We use "BytePtr" elsewhere, so we could call this IoctlSetIntPtr and just pretend IoctlSetPointerInt never happened.

@rsc rsc changed the title proposal: x/sys/unix: add IoctlSetModifiablePointerInt that reflects changes in the ioctl value proposal: x/sys/unix: add IoctlSetIntPtr May 12, 2021
@rsc rsc moved this from Incoming to Active in Proposals May 12, 2021
@rsc
Copy link
Contributor

@rsc rsc commented May 12, 2021

This proposal has been added to the active column of the proposals project
and will now be reviewed at the weekly proposal review meetings.
— rsc for the proposal review group

@rsc rsc changed the title proposal: x/sys/unix: add IoctlSetIntPtr proposal: x/sys/unix: add IoctlSetIntPtr May 12, 2021
@lhl2617
Copy link
Author

@lhl2617 lhl2617 commented May 13, 2021

We use "BytePtr" elsewhere, so we could call this IoctlSetIntPtr and just pretend IoctlSetPointerInt never happened.

Updated CL to reflect this. (https://golang.org/cl/318210)

@rsc
Copy link
Contributor

@rsc rsc commented May 19, 2021

Based on the discussion above, this proposal seems like a likely accept.
— rsc for the proposal review group

@rsc rsc moved this from Active to Likely Accept in Proposals May 19, 2021
@rsc
Copy link
Contributor

@rsc rsc commented May 26, 2021

No change in consensus, so accepted. 🎉
This issue now tracks the work of implementing the proposal.
— rsc for the proposal review group

@rsc rsc moved this from Likely Accept to Accepted in Proposals May 26, 2021
@rsc rsc changed the title proposal: x/sys/unix: add IoctlSetIntPtr x/sys/unix: add IoctlSetIntPtr May 26, 2021
@lhl2617
Copy link
Author

@lhl2617 lhl2617 commented May 28, 2021

Implemented in CL 318210

@lhl2617
Copy link
Author

@lhl2617 lhl2617 commented May 30, 2021

#45585 mentions the importance of differentiating between (u)int(32|64). On a 64 bit system ints in the kernel are usually 32 bits, so this function may be flaky since Go's ints may be 64 bits.

Perhaps what is required is
IoctlSet(Ui|I)nt(32|64)Ptr
Just to be more explicit about the int width.

Putting the CL as WIP now.

Thoughts?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Proposals
Accepted
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
3 participants