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

Add AIX support with fcntl #40

Merged
merged 1 commit into from
Aug 26, 2020
Merged

Add AIX support with fcntl #40

merged 1 commit into from
Aug 26, 2020

Conversation

Helflym
Copy link

@Helflym Helflym commented Apr 26, 2019

AIX doesn't provide a true flock() syscall. It does exist but it's just
a wrapper around fcntl. It doesn't provide safe locks under file
descriptors of a same process.

The current implementation is based on the file
cmd/go/internal/lockedfile/internal/filelock/filelock_fcntl.go.

Using fcntl implementation doesn't allow to have several RLocks at the
same time as closing a file descriptor might release the lock if even
others RLocks remain attached.

@theckman
Copy link
Member

theckman commented May 2, 2019

I plan to do a review of this with the intent to merge it. One concern I have is that without an AIX test environment, we can't be confident in our implementation when making future changes. I'm doing some research on that, but not looking promising. 😄

@Helflym
Copy link
Author

Helflym commented May 2, 2019

I'm not sure there is any "free" AIX which can be used for testing.
My aim is just to provide a "first port" hoping AIX users will get use to it and will fix it if anything ever go wrong.
Note that Solaris, according to cmd/go/internal/lockedfile/internal/filelock/filelock_fcntl.go should also use this fcntl version instead of flock version.

@Helflym
Copy link
Author

Helflym commented May 21, 2019

Hi Theckman,

Any news about this PR ?
Have you find any way to make tests on an AIX machine ?

flock_aix.go Outdated
Comment on lines 1 to 14
// Copyright 2019 Tim Heckman. All rights reserved. Use of this source code is
// governed by the BSD 3-Clause license that can be found in the LICENSE file.

// This code implements the filelock API using POSIX 'fcntl' locks, which attach
// to an (inode, process) pair rather than a file descriptor. To avoid unlocking
// files prematurely when the same file is opened through different descriptors,
// we allow only one read-lock at a time.
//
// This code is adapted from the Go package:
// cmd/go/internal/lockedfile/internal/filelock
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The portions of this implementation taken from the Go project should carry the Go license and copyright notice.

(It is of course fine to combine that code with other compatibly-licensed code, but it is important to comply with the terms of the license.)

@Helflym
Copy link
Author

Helflym commented Jan 3, 2020

Hi Theckman,

Any news about this PR ?
gofrs/flock is used in many others projects like Beats software and at the moment, we have to manually update "vendor" folder if we want to build them. I know that you can't test any future modifications on an AIX environment by yourself but I suppose letting the AIX community fixing it if it ever goes wrong is a viable solution

@fearful-symmetry
Copy link

@bcmills / @theckman Any update on this? We'd like this get this merged if possible. @Helflym should still be available but I can take over development if needed.

@bcmills
Copy link

bcmills commented Jul 21, 2020

@fearful-symmetry, I'm not in the gofrs organization. My involvement in this PR is purely as the maintainer of a Go package with similar functionality (namely, cmd/go/internal/lockedfile; see also golang/go#33974).

@fearful-symmetry
Copy link

@bcmills anyone I should ping about getting this updated/approved?

Copy link
Member

@theckman theckman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Helflym @fearful-symmetry @bcmills apologies, my hesitation has been that there are zero AIX build resources available for the community and it looked prohibitively expensive to try and learn AIX / gain access to one.

At this point I'm okay ignoring that discomfort and merging this in for the benefit of the community, but there is a blocker in getting this merged. There were some changes released in v0.7.2 (#43) that need to be included here and tested on an AIX system.

Is there someone with access to AIX hardware that can develop / test that change?

AIX doesn't provide a true flock() syscall. It does exist but it's just
a wrapper around fcntl. It doesn't provide safe locks under file
descriptors of a same process.

The current implementation is based on the file
cmd/go/internal/lockedfile/internal/filelock/filelock_fcntl.go.

Using fcntl implementation doesn't allow to have several RLocks at the
same time as closing a file descriptor might release the lock if even
others RLocks remain attached.
@Helflym
Copy link
Author

Helflym commented Aug 26, 2020

@Helflym @fearful-symmetry @bcmills apologies, my hesitation has been that there are zero AIX build resources available for the community and it looked prohibitively expensive to try and learn AIX / gain access to one.

Yeah, it's not really possible. IBM doesn't provide free AIX VM for OpenSource developers... As I said for other Go packages, the AIX community should report if anything go wrong and should be able to fix it. Anyway, you can tag me if you want some tests on AIX.

At this point I'm okay ignoring that discomfort and merging this in for the benefit of the community, but there is a blocker in getting this merged. There were some changes released in v0.7.2 (#43) that need to be included here and tested on an AIX system.

Is there someone with access to AIX hardware that can develop / test that change?

I've made the change it and the tests are all OK.

@theckman theckman merged commit 75ec202 into gofrs:master Aug 26, 2020
@theckman
Copy link
Member

@Helflym thank you! I'll work to get v0.8.0 out within the next 10-15 minutes.

@AlejandrosTesla8

This comment was marked as off-topic.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants