Navigation Menu

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: export Fcntl and make more POSIX compliant #24677

Closed
ppbrown opened this issue Apr 4, 2018 · 5 comments
Closed

x/sys/unix: export Fcntl and make more POSIX compliant #24677

ppbrown opened this issue Apr 4, 2018 · 5 comments
Labels
FrozenDueToAge NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made.
Milestone

Comments

@ppbrown
Copy link

ppbrown commented Apr 4, 2018

go version go1.10 linux/amd64

This is related to issue #24649 and thus to code review
https://go-review.googlesource.com/c/sys/+/104255

I am going to be attaching a new code PR to this issue. If accepted, I can then cancel the prior one.

It was discussed in 104255, how the potentially most clean approach is to finally export fcntl as Fcntl.

Unfortunately, the existing x/sys/unix.fcntl is not standards compliant with its type.
The third argument is declared as "int": however, sometimes, fcntl calls require the third argument to be a pointer.
This mandates that the third argument be changed to uintptr, rather than int.
Happily, it is currently an internal function only, so making the change does not require too many cascading updates.

@gopherbot gopherbot added this to the Unreleased milestone Apr 4, 2018
@ppbrown
Copy link
Author

ppbrown commented Apr 4, 2018

Good news:
I have the new code, written, TESTED, and ready to commit.

Problem:
This touches multiple files, in small ways.
I made separate "git commit" calls locally, so that the details of the individual files would get commented.
Then tried to do a unified "git change".
But it wont let me do that.

git-codereview: cannot amend change: multiple changes pending:
fd3cb4e update type of fcntl to properly have 3rd arg as uintptr, to be fully standard compliant
b33df1f update type of fcntl to properly have 3rd arg as uintptr, to be fully standard compliant
8f2c662 New test based around flock_test.go, but using Fcntl directly, rather than FcntlFlock
2594872 Exported fcntl as Fcntl
4a0a565 Updated third arg to fcntl to match new definition (int -> uintptr)

So, how should I proceed?
It feels wrong to me to submit each of these file changes as a separate change, since if for example, only one were accepted, it would BREAK builds, until the rest were accepted.

I am trying to follow "Multiple-Commit Work Branches" paragraph in the git-codereview docs, but it seems to be missing a few steps.
I've even tried ensuring that the Change-Id value is the same on all commits.. but it still refuses to allow me to run "git-codereview change".

??

@ppbrown
Copy link
Author

ppbrown commented Apr 4, 2018

PS: just so I dont forget, I'm noting here that if my code changes are accepted, there will need to be a robo-auto-update, to fix the autogenerated zsyscall_xxx files (since I was informed that may only happen on the official build farm)

@ppbrown ppbrown changed the title x/sys/unix: export Fcntl and make more POSIX compliant x/sys/unix: here is code to export Fcntl and make more POSIX compliant Apr 4, 2018
@tklauser tklauser added the NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. label Apr 4, 2018
@tklauser
Copy link
Member

tklauser commented Apr 4, 2018

(Pasting from my reply in https://golang.org/cl/104255)

After reading https://golang.org/pkg/unsafe/#Pointer and checking on the discussion around the introduction of the Ioctl* functions (which are quite similar in the way they work), I think we shouldn't add a raw exported Fcntl at all. Regardless of whether arg is int or uintptr, users would have to use unsafe.Pointer if they want to pass pointers.

E.g. see the discussion in https://golang.org/cl/44009 and #12574, especially Ian's comments #12574 (comment) and #12574 (comment).

Thus, I'd propose to add Fcntl* functions for each specific type (we already have FcntlFlock anyway) which will use the currently existing fcntl function. We might want to change its implementation to use uintptr arg instead of int.

This would allow users to use these functions in a type-safe manner without using the unsafe package.

@bradfitz, @ianlancetaylor what do you think?

@bradfitz
Copy link
Contributor

bradfitz commented Apr 4, 2018

Agree. Specific type-safe fnctl sounds good.

@gopherbot
Copy link

Change https://golang.org/cl/104736 mentions this issue: x/sys/unix: add FcntlInt

@bcmills bcmills changed the title x/sys/unix: here is code to export Fcntl and make more POSIX compliant x/sys/unix: export Fcntl and make more POSIX compliant Apr 4, 2018
@golang golang locked and limited conversation to collaborators Apr 4, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made.
Projects
None yet
Development

No branches or pull requests

4 participants