fuse t0030-mount.sh regression

[chriscool]

These days when running the sharness tests on my Linux machine, I always get the following test failure:

expecting success: 
    test_must_be_empty output &&
    test_should_contain "not_ipns\|not_ipfs" output.err

'output.err' does not contain 'not_ipns\|not_ipfs', it contains:
Error: fusermount: exit status 1
not ok 7 - 'ipfs mount' output looks good
#
#               test_must_be_empty output &&
#               test_should_contain "not_ipns\|not_ipfs" output.err
#

Bisecting this failure points to the following commit that updates a dependency:

commit e9074beb6da592a203ddcea3e4a22dd9c0af13fb Author: Henry cryptix@riseup.net Date: Thu May 28 22:09:26 2015 +0200

godeps: update bazil.org/fuse

fuse: Attr() now has a Context parameter and error return value

~GOPATH/src/bazil.org/fuse:master$ git shortlog 48c34fb7780b88aca1696bf865508f6703aa47f1..e4fcc9a2c7567d1c42861deebeb483315d222262
Tommi Virtanen (8):
      Remove dead code
      Make saveLookup take Context, return error
      Make serveNode.attr take Context, return error
      Make nodeAttr take Context, return error
      API change: Move attribute validity time inside Attr
      Set attribute validity default time in one place
      API change: Attr method takes Context, returns error
      Set LookupResponse validity times up front, instead of after the handler

cc @cryptix @tv42

[tv42]

That's a race, it's likely unmounting too soon and the mount point is still busy. The commit involved should have no real change on the behavior, apart from the race coming & going as a random side effect.

In bazil.org/fuse's own unit tests, I do this https://github.com/bazil/fuse/blob/4fe24646670059b759e9a2b06a37384b7877f166/fs/fstestutil/mounted.go#L37 and regularly see a few of those messages, with the loop retrying 0-2 times before the unmount succeeds.

I am not aware of any mechanism to wait until a mount point is not-busy.

[chriscool]

The test in t0030 is the following:

test_expect_success "'ipfs mount' fails when there is no mount dir" '
    tmp_ipfs_mount() { ipfs mount -f=not_ipfs -n=not_ipns >output 2>output.err; } &&
    test_must_fail tmp_ipfs_mount
'

test_expect_success "'ipfs mount' output looks good" '
    test_must_be_empty output &&
    test_should_contain "not_ipns\|not_ipfs" output.err
'

So we are checking that ipfs mount will report an error like "cannot mount not_ipfs" when it cannot mount because the specified directory does not exist. Instead we get "Error: fusermount: exit status 1". So we get an error but it's not a nice error for the user.

It doesn't look like a race problem for me and it doesn't look like an unmounting problem.

[tv42]

Oh sorry, I jumped the gun. I usually see fusermount complaining at unmount time, and have seen that with sharness.

[jbenet]

• Looks like this error changed in the new fuse version: https://github.com/ipfs/go-ipfs/blob/c73d0e0b5674dc8c6eb87d516c90f775705e0316/core/commands/mount_unix.go#L184-L192
• seems like the double "fusermount:" message is no longer there, and it no longer spits out the name of the directory. fuse just reports (as @chriscool pointed out) fusermount: exit status 1. Is this intended @tv42 ?

---

• test_expect_success "'ipfs mount' fails when there is no mount dir" ' works ok in osx
• test_expect_success "'ipfs mount' output looks good" ' works ok in osx
• important to make sure to run all the tests when updating deps. (cc @cryptix)
• we should try to make our CI systems run fuse tests too

[tv42]

I really have no idea what that test wants to test, but it seems to testing some detail of error message content that's in no way guaranteed.

[jbenet]

@tv42

• the test is testing that the error returned to the user includes the name of the path unable to be mounted to. this ensures users have a better UX when things go wrong.
• not testing error messages makes for a poor UX.

what i'm asking you is whether you intend for the message to have changed. fuse used to return an error like

fusermount: "fusermount: <msg-including-path>" exit status 1

so we removed the wrapping and returned the raw <msg-including-path>.

do your latest changes intend for the (helpful) <msg-including-path> to be replaced by the (obscure) fusermount: exit status 1?? (If so, we'll craft our own helpful message another way, but just checking because you may want to have a better UX there.)

[tv42]

That's /bin/fusermount not my code.

[jbenet]

ok we'll fix it on our end-- sorry to bother :)

[tv42]

Well, it's more, fusermount doesn't give a nice error to stderr for me to use.

https://github.com/bazil/fuse/commit/d28b015fbc4824368118e16ce3a1a11f4979b3dd switched from capturing stderr and returning it as an error to just logging them, as fusermount was observed sending frivolous messages to stderr.

I'm sure that could be done somehow better, but there's bigger fish to fry for now.