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

syscall: remove AIX implementation of Flock #29084

Closed
bcmills opened this issue Dec 3, 2018 · 15 comments
Closed

syscall: remove AIX implementation of Flock #29084

bcmills opened this issue Dec 3, 2018 · 15 comments
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done. release-blocker
Milestone

Comments

@bcmills
Copy link
Contributor

bcmills commented Dec 3, 2018

https://golang.org/cl/138720 added an AIX implementation of syscall.Flock using the fcntl system call.

We rejected a similar implementation for Solaris in #24684 on the grounds that fcntl and flock are semantically different APIs (see #24684 (comment)).

(Note that per #21410 (comment), some versions of Illumos — a Solaris variant — also support a proper flock implementation: https://www.illumos.org/issues/3252.)

We should remove the AIX Flock implementation before the 1.12 release. If we decide to add it later, we should ensure a consistent approach between Solaris and AIX.

(CC @Helflym @ianlancetaylor @rsc @tklauser)

@bcmills bcmills added NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. release-blocker labels Dec 3, 2018
@bcmills bcmills added this to the Go1.12 milestone Dec 3, 2018
@bcmills
Copy link
Contributor Author

bcmills commented Dec 3, 2018

...or did Flock already get released in 1.11?

@Helflym
Copy link
Contributor

Helflym commented Dec 3, 2018

AIX has a flock syscall (cf https://www.ibm.com/support/knowledgecenter/en/ssw_aix_72/com.ibm.aix.basetrf1/lockfx.htm) and it also calls fcntl almost as it is done in the syscall package.

@Helflym
Copy link
Contributor

Helflym commented Dec 3, 2018

However, I do understand that using a fake Flock can be confusing.
@bcmills it's already released in the gcccgo frontend. But a lot of thing has changed since the port of aix/ppc64 on the gc version. So I don't think it really matters.

@bcmills
Copy link
Contributor Author

bcmills commented Dec 3, 2018

That page says:

The flock subroutine locks and unlocks entire files. This is a limited interface maintained for BSD compatibility, although its behavior differs from BSD in a few subtle ways.

The existence of a C compatibility shim is not, to my mind, a good reason to have the same compatibility shim in Go, given that portable Go programs already need to distinguish between a proper BSD flock and a POSIX fcntl (at least if they want to build on solaris).

And Go 1 compatibility makes it much easier to add functions in the future than to remove them: I think we should remove it for now, and we can always consider adding it back in the future if there is a compelling need.

(This sort of shim might be better suited to golang.org/x/sys/unix, which IIRC does not have the same compatibility constraints as the standard-library syscall package.)

@Helflym
Copy link
Contributor

Helflym commented Dec 3, 2018

Yes, I do agree, AIX isn't perfect on BSD syscalls... Flock isn't needed anywhere inside the stdlib expect for cmd/go/internal/lockedfile/internal/filelock where AIX can use the Solaris version with FcntlFlock. So I think it can be removed without any problems.

@tklauser
Copy link
Member

tklauser commented Dec 3, 2018

(This sort of shim might be better suited to golang.org/x/sys/unix, which IIRC does not have the same compatibility constraints as the standard-library syscall package.)

Removing Flock for aix from syscall SGTM. We can still add it to x/sys/unix in case something outside the Go standard library needs it.

@bcmills bcmills added the NeedsFix The path to resolution is known, but the work has not been done. label Dec 3, 2018
@gopherbot gopherbot removed the NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. label Dec 3, 2018
@bcmills
Copy link
Contributor Author

bcmills commented Dec 3, 2018

@Helflym is planning to send a CL to move filelock over to fcntl (#29065 (comment)). One or the other of us can remove Flock once that lands (or even in the same CL).

@tklauser
Copy link
Member

tklauser commented Dec 3, 2018

Great, thanks. Let's wait for that CL to land.

@gopherbot
Copy link

Change https://golang.org/cl/152397 mentions this issue: syscall, cmd/go/internal/lockedfile: remove Flock syscall for aix/ppc64

@tklauser
Copy link
Member

tklauser commented Dec 4, 2018

it's already released in the gcccgo frontend.

There seems to be no GCC release with this gccgo change yet. Should we remove syscall.Flock for aix from gccgo as well so it is consistent with gc?

/cc @ianlancetaylor

@tklauser
Copy link
Member

tklauser commented Dec 4, 2018

There seems to be no GCC release with this gccgo change yet.

Sorry, please disregard - I checked in an outdated GCC copy. GCC 8.1 and GCC 8.2 already contain the change, so I guess we cannot remove it.

@Helflym
Copy link
Contributor

Helflym commented Dec 4, 2018

By the way, since Solaris, AIX or Windows can't use Flock, a user will have to create a lockfile_unix.go, lockfile_fcntl.go and so on if he wants to create a package with a filelock feature as it's done inside cmd/go/internal/lockedfile.
Should not Go provide such functions inside os package ? I would be better than to have all these files which might not work under specific OS.

@bcmills
Copy link
Contributor Author

bcmills commented Dec 4, 2018

Should not Go provide such functions inside os package?

I think filelock is too subtle for the standard library. However, I will probably propose moving lockedfile to the standard library for 1.13 or 1.14.

(Probably best to give it a release or two to work out the rough edges before we expose it to the world — and to Go 1 compatibility.)

@ianlancetaylor
Copy link
Contributor

It's OK to remove from gccgo. In practice programs expect the gc toolchain's syscall package, so it's OK to remove names from gccgo's syscall package if they aren't in gc's package.

@gopherbot
Copy link

Change https://golang.org/cl/152557 mentions this issue: syscall: remove Flock for aix/ppc64

gopherbot pushed a commit to golang/gofrontend that referenced this issue Dec 5, 2018
CL 152397 removed it from gc's syscall package.

Updates golang/go#29084

Change-Id: Iac26d912851cdf34c208b9b7eb02b522c085c5ef
Reviewed-on: https://go-review.googlesource.com/c/152557
Reviewed-by: Ian Lance Taylor <iant@golang.org>
kraj pushed a commit to kraj/gcc that referenced this issue Dec 5, 2018
    
    CL 152397 removed it from gc's syscall package.
    
    Updates golang/go#29084
    
    Reviewed-on: https://go-review.googlesource.com/c/152557


git-svn-id: svn+ssh://gcc.gnu.org/svn/gcc/trunk@266812 138bc75d-0d04-0410-961f-82ee72b054a4
@golang golang locked and limited conversation to collaborators Dec 5, 2019
@golang golang unlocked this conversation Jan 10, 2024
@golang golang locked as resolved and limited conversation to collaborators Jan 10, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done. release-blocker
Projects
None yet
Development

No branches or pull requests

5 participants