implement file locking: getlk/setlk/setlkw/flock #199

kaoet · 2018-01-16T07:28:21Z

googlebot · 2018-01-16T07:28:25Z

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed, please reply here (e.g. I signed it!) and we'll verify. Thanks.

If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address on your commit. Check your existing CLA data and verify that your email is set on your git commits.
If your company signed a CLA, they designated a Point of Contact who decides which employees are authorized to participate. You may need to contact the Point of Contact for your company and ask to be added to the group of authorized contributors. If you don't know who your Point of Contact is, direct the project maintainer to go/cla#troubleshoot. The email used to register you as an authorized contributor must be the email used for the Git commit.
In order to pass this check, please resolve this problem and have the pull request author add another comment and the bot will run again. If the bot doesn't comment, it means it doesn't think anything has changed.

kaoet · 2018-01-16T07:38:34Z

I signed it!

googlebot · 2018-01-16T07:38:38Z

CLAs look good, thanks!

hanwen · 2018-01-16T08:31:51Z

fuse/nodefs/api.go

@@ -102,6 +102,11 @@ type Node interface {
 	SetXAttr(attr string, data []byte, flags int, context *fuse.Context) fuse.Status
 	ListXAttr(context *fuse.Context) (attrs []string, code fuse.Status)

+	// File locking
+	GetLk(file File, owner uint64, lk *fuse.FileLock, flags uint32, padding uint32, out *fuse.FileLock, context *fuse.Context) (code fuse.Status)


padding should never be part of function signatures.

hanwen · 2018-01-16T08:35:26Z

fuse/test/file_lock_test.go

@@ -14,8 +14,7 @@ import (
 	"testing"
 )

-// See https://github.com/hanwen/go-fuse/issues/170
-func disabledTestFlock(t *testing.T) {
+func TestFlock(t *testing.T) {


this test was a nice try, but I don't trust it. Can you make something that implements a lock, and verifies that the user-space locking code gets called?

mariash · 2018-03-16T19:22:38Z

Hi, we also tested this with nfs v4 and locking works, so we would appreciate if this PR would be merged.

hanwen · 2018-03-17T12:28:51Z

Hi! Thanks for reminding me. I'll have a look this week Op 16 mrt. 2018 20:22 schreef "Maria Shaldibina" <[email protected]>:

…

Hi, we also tested this with nfs v4 and locking works, so we would appreciate if this PR would be merged. — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#199 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAB7O7CJOdZT9TyYPkVtzJiNkoac9u5pks5tfBD-gaJpZM4RfWdj> .

hanwen · 2018-03-18T12:19:35Z

fuse/nodefs/files.go

-	r := fuse.ToStatus(syscall.Flock(int(f.File.Fd()), flags))
-	f.lock.Unlock()
+const (
+	F_OFD_GETLK  = 36


this looks like a syscall interface. Is this the same on OSX ? I would expect this defs to be a linux-specific file (I assume you wrote and tested this under linux.)

We tested these constant on OSX and they seem to be working.

hanwen · 2018-03-18T12:23:30Z

fuse/nodefs/api.go

@@ -102,6 +102,11 @@ type Node interface {
 	SetXAttr(attr string, data []byte, flags int, context *fuse.Context) fuse.Status
 	ListXAttr(context *fuse.Context) (attrs []string, code fuse.Status)

+	// File locking


I know this wasn't documented well before, but could you add some more wording here? I guess Lkw locks for write and Lk is a concurrent (?) read lock? If you don't want to write docs, add a manpage reference.

hanwen · 2018-03-18T12:42:18Z

fuse/nodefs/files.go

+		default:
+			return fuse.EINVAL
+		}
+		if !blocking {


if this is controlled from a field in the flags, why have the separate argument 'blocking' ?

Given this signature:

setLock(owner uint64, lk *fuse.FileLock, flags uint32, blocking bool)

we understand your comment to mean that flags will indicate whether the lock call should be blocking, or not.

So we tested this with the loopback mounter and flock, running:

flock -n /tmp/mnt1/foo -c 'echo bar'

and

flock /tmp/mnt1/foo -c 'echo bar'

and in fact

flock -x /tmp/mnt1/foo -c 'echo bar'

In all three cases the lk.Typ and flags were 0x1.

It is entirely possible that we have misunderstood. We also haven't tested with fcntl yet either but maybe this is why @kaoet added a blocking flag. I think we agree that if flags does indicate blocking then it is better to use those than duplicate the intention with an additional boolean but we can't see how it does. What are we missing?

blocking is determined by SetLk vs SetLkw

Since it is determined by which one of the methods was called (SetLk or SetLkw) and flags do not indicate whether it is blocking we have to pass a separate argument to setLock method. Or we can collapse setLock method into SetLk and SetLkw if that makes it more readable?

hanwen · 2018-03-18T12:43:42Z

fuse/test/file_lock_test.go

@@ -0,0 +1,125 @@
+// Copyright 2016 the Go-FUSE Authors. All rights reserved.


2018

did you add your name to the AUTHORS file?

Not sure who he is but we added his name to the authors file on his behalf. Hope that's ok with you Kaoet?

I missed update to AUTHORs in your commit?

hanwen · 2018-03-18T12:44:43Z

fuse/test/file_lock_test.go

+	}
+
+	if out, err := runExternalFlock(cmd, tc.mountFile); !bytes.Contains(out, []byte("failed to get lock")) {
+		t.Errorf("runExternalFlock(%q): %s (%v)", tc.mountFile, out, err)


use %q iso. %s. For []byte %s will use [1 2 3] as formatting.

This test was slightly changed. Since on Ubuntu Trusty flock does not have a verbose option we can't assert an output. So we relaxed an assertion that command fails which in turn removed this Errorf line.

https://github.com/hanwen/go-fuse/pull/220/files#diff-49306d0ec901b97786aec218f18ee349R21

hanwen · 2018-03-18T12:46:52Z

fuse/test/file_lock_test.go

+	defer f.Close()
+	cmd := exec.Command(flockPath, "--verbose", "--exclusive", "--nonblock", "3")
+	cmd.Env = append(cmd.Env, "LC_ALL=C") // in case the user's shell language is different
+	cmd.ExtraFiles = []*os.File{f}


this needs some more comment. Why do you not pass the filename to the flock command line?

added in our PR https://github.com/hanwen/go-fuse/pull/220/files#diff-49306d0ec901b97786aec218f18ee349R56

hanwen · 2018-03-18T12:47:18Z

fuse/test/file_lock_test.go

+
+	node := &lockingNode{
+		Node:    nodefs.NewDefaultNode(),
+		flockInvoked: false,


drop (is implicit)

Can you clarify? By drop (is implicit) do you mean that flockinvoked doesn't need to be set to false because that's the default for a bool or ???

correct. The default value is false, so no need to mention it.

done in our PR https://github.com/hanwen/go-fuse/pull/220/files#diff-49306d0ec901b97786aec218f18ee349R117

hanwen · 2018-03-18T12:48:07Z

fuse/test/file_lock_test.go

+	root.Inode().NewChild("foo", false, node)
+
+	realPath := filepath.Join(dir, "foo")
+	cmd:=exec.Command(flock, "--nonblock", realPath, "echo", "locked")


did you run gofmt?

hanwen · 2018-03-18T12:53:48Z

fuse/test/file_lock_test.go

+	}
+	if !node.flockInvoked {
+		t.Fatalf("flock is not invoked")
+	}


this test makes sure the locking really goes through FUSE (which is good), but I can't see the functionality of GetLk/SetLk/SetLkw tested separately. From the signatures, it seems one can have different locks on ranges of the file, and it would be good to exercise that too. Can you think of a way to add coverage for those features? Maybe you could record the ranges of the locks and verify they are as you expect?

We added additional testing to ensure SetLk and SetLkw are called but we couldn't find a way to use flock to invoke GetLk. We tried cgo in an attempt to call fcntl.c but cgo isnt allowed in test files. Maybe you have some other ideas?

#220

kaoet · 2018-07-09T04:01:51Z

Let's use @paulcwarren 's PR. #220

hanwen reviewed Jan 16, 2018

View reviewed changes

implement file locking: getlk/setlk/setlkw/flock

9e12134

hanwen requested changes Mar 18, 2018

View reviewed changes

paulcwarren mentioned this pull request Jul 3, 2018

Updating kaoet's PR with your feedback #220

Closed

kaoet closed this Jul 9, 2018

kaoet deleted the lock branch July 9, 2018 04:01

hanwen mentioned this pull request Dec 27, 2020

flock implementation is untested #170

Closed

		@@ -0,0 +1,125 @@
		// Copyright 2016 the Go-FUSE Authors. All rights reserved.

implement file locking: getlk/setlk/setlkw/flock #199

implement file locking: getlk/setlk/setlkw/flock #199

Conversation

kaoet commented Jan 16, 2018

googlebot commented Jan 16, 2018

kaoet commented Jan 16, 2018

googlebot commented Jan 16, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mariash commented Mar 16, 2018

hanwen commented Mar 17, 2018 via email

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

paulcwarren Jul 3, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mariash Jul 19, 2018 • edited Loading

Choose a reason for hiding this comment

kaoet commented Jul 9, 2018

paulcwarren Jul 3, 2018 •

edited

Loading

mariash Jul 19, 2018 •

edited

Loading