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

WIP Feature/macos compatibility #11

Closed
wants to merge 3 commits into from

Conversation

yemartin
Copy link

@yemartin yemartin commented Jun 9, 2019

Do not merge yet:

The current workaround for the MacOS + SMB bug works, but it causes an error to be displayed:

$ cshatag test.bin
<outdated> test.bin
 stored: 0000000000000000000000000000000000000000000000000000000000000000 0000000000.000000000
 actual: e3b0c44298fc1c149afbf4c8996fb92427ae41e4649b934ca495991b7852b855 1560090071.529093717
Error: could not write extended attributes to file "test.bin": Attribute not found

My C is really rusty... Can you suggest a better way to implement it?

Use precompiler macros to transparently patch in MacOS support.
The differences are:

- the `sys/stat` modification time struct
- the `fgetxattr` and `fsetxattr` function signatures
- the "attribute not found" error
When an outdated tag is found on an SMB filesystem on a Mac, it seems the call
to `fsetxattr` does not update the xattr but removes it instead. So it takes
two runs of `cshatag` to update the attribute. To work around this issue, we
remove the xattr explicitely before setting it again.
Copy link

@codelingo codelingo bot left a comment

Choose a reason for hiding this comment

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

LGTM

@rfjakob
Copy link
Owner

rfjakob commented Jun 9, 2019

Can you check where the error comes from? (from fremovexattr or from fsetxattr?)

The previous workaround was working, but it was causing an error to be displayed
when adding a new tag, since the call to `fremovexattr` would fail with
`ENOATTR`. Two calls to `fsetxattr` in a row also work around the bug, this time
without displaying an error.
Copy link

@codelingo codelingo bot left a comment

Choose a reason for hiding this comment

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

LGTM

@yemartin
Copy link
Author

yemartin commented Jun 9, 2019

The error comes from the fremovexattr, failing with ENOATTR when the file did not already have a tag. I guess that makes sense.

Maybe we could just ignore that error, but I don't know how to do that using a preprocessor macro since the macro also needs to be a single statement that returns the return value of the fsetxattr call...

Of course, I could write a small function to do it, but I don't like the idea of having to pepper #ifdef __APPLE__ in the middle of the code. I really like how the MacOS compatibility is currently simply preprocessor macros.

I found out that two calls to fremovexattr fix the issue and also do not return an error. I have updated the PR accordingly. Please let me know what you think.

@rfjakob
Copy link
Owner

rfjakob commented Jun 10, 2019

@yemartin I'm thinking about rewriting the whole thing in Go, using the https://github.com/pkg/xattr library, which works out of the box on Linux, MacOS and FreeBSD.

The goal is to support passing multiple files per invocation, which should speed up processing of lots of small files considerably, and also to have command-line flags. I was thinking about you ticket #9 about not updating the tag when corruption is found. I think both behavoirs can make sense, so I would add a -f flag to get the the old behavoir back.

@rfjakob
Copy link
Owner

rfjakob commented Jun 10, 2019

I wondered if you could test a Go rewrite on MacOS?

@yemartin
Copy link
Author

@rfjakob Great idea, always better to use existing packages rather than reinventing the wheel. And of course I would be more than happy to test a Go rewrite. Do you have a repo for it already?

@rfjakob
Copy link
Owner

rfjakob commented Jun 10, 2019

I do. You can

git pull
git checkout golang-rewrite
make

to get the new version.

@yemartin
Copy link
Author

Building

@rfjakob Maybe this is basic stuff, but I am new to Go and the package did not build out of the box, with a cannot find package "github.com/pkg/xattr" error. After a little googling I did the following, which allowed me to build but gave me a warning that I am not sure it is OK to ignore or not:

$ go get ./...
go get: no install location for directory /Users/yemartin/Documents/GitHub/cshatag outside GOPATH
	For more details see: 'go help gopath'

Anyway, make succeeded after that.

Running

On my local Mac filesystem, everything seems to be working fine:

  • Adding new tag to file => OK
  • Testing valid tag => OK
  • Updating outdated tag => OK
  • Detecting corruption => OK
  • Detecting corruption does NOT update the tag => OK 👍

About the MacOS + SMB bug

Using my oneliner test from #8 (comment) the output on my local filesystem is:

<outdated> test.bin
 stored: 0000000000000000000000000000000000000000000000000000000000000000 0000000000.000000000
 actual: e3b0c44298fc1c149afbf4c8996fb92427ae41e4649b934ca495991b7852b855 1560201866.922689645
<outdated> test.bin
 stored: e3b0c44298fc1c149afbf4c8996fb92427ae41e4649b934ca495991b7852b855 1560201866.922689645
 actual: b5bb9d8014a0f9b1d61e21e796d78dccdf1352f23cd32812f4850b878ae4944c 1560201869.952861502
<ok> test.bin

Unfortunately, the bug is still present when connecting to my SMB_3.02 NAS share:

<outdated> test.bin
 stored: 0000000000000000000000000000000000000000000000000000000000000000 0000000000.000000000
 actual: e3b0c44298fc1c149afbf4c8996fb92427ae41e4649b934ca495991b7852b855 1560201880.000000000
<outdated> test.bin
 stored: e3b0c44298fc1c149afbf4c8996fb92427ae41e4649b934ca495991b7852b855 1560201880.000000000
 actual: b5bb9d8014a0f9b1d61e21e796d78dccdf1352f23cd32812f4850b878ae4944c 1560201883.000000000
<outdated> test.bin
 stored: 0000000000000000000000000000000000000000000000000000000000000000 0000000000.000000000
 actual: b5bb9d8014a0f9b1d61e21e796d78dccdf1352f23cd32812f4850b878ae4944c 1560201883.000000000

So with MacOS + SMB, trying to update a tag still results in clearing it instead.

To research this further, I changed the min/max version of SMB on my NAS for force a given protocol, and here are my results:

SMB_1:     OK
SMB_2.002: BUG
SMB_2.1:   BUG
SMB_3.02:  BUG

So, the root cause of this bug is probably totally outside of cshatag, but it is still something that we may have to work around.

@yemartin
Copy link
Author

One more test:

Performance

5 runs of time (for run in {1..100}; do cshatag ./test.bin >/dev/null; done) on a 50MB test file:

  • C version (using my feature/macos_compatibility branch):
14.29s user 1.27s system 96% cpu 16.058 total
14.08s user 1.24s system 97% cpu 15.789 total
14.06s user 1.24s system 97% cpu 15.741 total
14.03s user 1.23s system 97% cpu 15.711 total
14.03s user 1.24s system 97% cpu 15.723 total
  • Go version:
14.10s user 1.67s system 100% cpu 15.663 total
13.84s user 1.41s system 100% cpu 15.157 total
13.91s user 1.41s system 100% cpu 15.229 total
14.21s user 1.52s system 100% cpu 15.627 total
13.89s user 1.42s system 100% cpu 15.227 total

So pretty much the same. Interesting how Go uses 100% CPU vs. 97% for C, but is also (maybe as a result) a tiny bit faster.

@rfjakob
Copy link
Owner

rfjakob commented Jun 11, 2019

I just pushed a commit that adds the "remove xattr before setting" workaround. Can you git pull ; make and check again? The compile warning seems harmless.

@yemartin
Copy link
Author

Sorry about the delay @rfjakob: I have had some issues with my NAS and could not do any testing last week. Things should be back in order now, I will give it a try tonight.

@yemartin
Copy link
Author

yemartin commented Jun 18, 2019

Two good news:

  1. I confirmed the Mac + SMB bug outside of cshatag:

On the Mac, inside an SMB-shared folder:

$ touch file.bin

$ xattr -w user.test value1 file.bin
$ xattr -l file.bin
user.test: value1

$ xattr -w user.test value2 file.bin
$ xattr -l file.bin
                              # <-------- The tag was removed!

$ xattr -w user.test value2 file.bin
$ xattr -l file.bin
user.test: value2             # <--------- Now it is back
  1. The workaround in the latest golang-rewrite worked like a charm!

The output of my little one-liner (see #8 (comment)) is:

$ rm test.bin; \
touch test.bin \
&& cshatag test.bin \
&& sleep 3 \
&& echo "foo" >> test.bin \
&& cshatag test.bin \
&& cshatag test.bin

remove test.bin? y
<outdated> test.bin
 stored: 0000000000000000000000000000000000000000000000000000000000000000 0000000000.000000000
 actual: e3b0c44298fc1c149afbf4c8996fb92427ae41e4649b934ca495991b7852b855 1561414431.000000000
<outdated> test.bin
 stored: e3b0c44298fc1c149afbf4c8996fb92427ae41e4649b934ca495991b7852b855 1561414431.000000000
 actual: b5bb9d8014a0f9b1d61e21e796d78dccdf1352f23cd32812f4850b878ae4944c 1561414434.000000000
<ok> test.bin

UPDATED: Fixed the oneliner output

@yemartin
Copy link
Author

I just realized I must have pasted the wrong output. The end result regarding the workaround is the same <ok>, but there is a small difference that lead me to a new issue: time stamps precision. I filed it as a new issue since it is unrelated (could happen with not just SMB but also FAT, is not Mac related, and could be implemented in the C or Go versions): Issue #12

@rfjakob
Copy link
Owner

rfjakob commented Nov 17, 2019

I understand that this is fixed, closing in favor of #12

@rfjakob rfjakob closed this Nov 17, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants