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

Comments

Projects
None yet
5 participants
@bcmills
Copy link
Member

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

This comment has been minimized.

Copy link
Member

bcmills commented Dec 3, 2018

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

@Helflym

This comment has been minimized.

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

This comment has been minimized.

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

This comment has been minimized.

Copy link
Member

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

This comment has been minimized.

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

This comment has been minimized.

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 label Dec 3, 2018

@gopherbot gopherbot removed the NeedsDecision label Dec 3, 2018

@bcmills

This comment has been minimized.

Copy link
Member

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

This comment has been minimized.

Copy link
Member

tklauser commented Dec 3, 2018

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

@gopherbot

This comment has been minimized.

Copy link

gopherbot commented Dec 4, 2018

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

@tklauser

This comment has been minimized.

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

This comment has been minimized.

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

This comment has been minimized.

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.

@gopherbot gopherbot closed this in 7937466 Dec 4, 2018

@bcmills

This comment has been minimized.

Copy link
Member

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

This comment has been minimized.

Copy link
Contributor

ianlancetaylor commented Dec 4, 2018

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

This comment has been minimized.

Copy link

gopherbot commented Dec 5, 2018

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

syscall: remove Flock for aix/ppc64
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

ian
syscall: remove Flock for aix/ppc64
    
    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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment