Arvados

Reviewing 98acd11. Unfortunately, this branch does not implement the story as specified.

Please add the specified tests. In case you didn't know, exit and sys.exit just raise a SystemExit exception. Tests could mock or reconfigure things to intentionally fail, then call the main method and assert it raises SystemExit. The exception's code attribute will be the exit code.

Please construct the range as specified, and use the system's rlimit as the end, rather than an arbitrary constant. You want resource.getrlimit(resource.RLIMIT_NOFILE).

Please always pass the range as preserve_files, and don't go searching for the FUSE file descriptor. The current branch breaks --logfile when daemonizing, because it doesn't pass the logfile file descriptor in preserve_files. You could manually add that to the current code, of course, but that just means that someone else is likely to trip up on this if they add a feature that opens a file descriptor in the future. Like the description says, "Anything arv-mount opens, we trust it to need after daemonization." Doing the naïve thing is less code, meaning less bugs, and more future-proof.

Thanks.

Brett Smith wrote:

Reviewing 98acd11. Unfortunately, this branch does not implement the story as specified.

Please add the specified tests. In case you didn't know, exit and sys.exit just raise a SystemExit exception. Tests could mock or reconfigure things to intentionally fail, then call the main method and assert it raises SystemExit. The exception's code attribute will be the exit code.

Done.

Please construct the range as specified, and use the system's rlimit as the end, rather than an arbitrary constant. You want resource.getrlimit(resource.RLIMIT_NOFILE).

Please always pass the range as preserve_files, and don't go searching for the FUSE file descriptor. The current branch breaks --logfile when daemonizing, because it doesn't pass the logfile file descriptor in preserve_files. You could manually add that to the current code, of course, but that just means that someone else is likely to trip up on this if they add a feature that opens a file descriptor in the future. Like the description says, "Anything arv-mount opens, we trust it to need after daemonization." Doing the naïve thing is less code, meaning less bugs, and more future-proof.

Done.

Now at d7d291b

Reviewing d7d291b, and this branch is good. Thanks for all the follow-ups. The tests especially look nice.

There's one functional change I'm wondering about before merge: master uses self.args.mountpoint as the working directory for the daemon. This branch makes it dirname(self.args.mountpoint). Was this an intentional change? If so, what's the rationale behind it?

Beyond that, just a couple of small suggestions for niceties:

It would be nice to add a comment above the new users/current API call to explain why we're doing it. Otherwise it can look a lot like a noop to future readers.

In test_bogus_host, '100::' would be a better bogus host. The RFC says that traffic to 100::/64 must be immediately refused, so this way the test can't pass in case a local resolver gives an answer for 'example.null' or anything like that.

Thanks.

Brett Smith wrote:

Reviewing d7d291b, and this branch is good. Thanks for all the follow-ups. The tests especially look nice.

There's one functional change I'm wondering about before merge: master uses self.args.mountpoint as the working directory for the daemon. This branch makes it dirname(self.args.mountpoint). Was this an intentional change? If so, what's the rationale behind it?

Yes. What happens is that before it would change into the mount directory prior to mounting meant the CWD of the daemon was associated with the inode of empty mount point on the host filesystem; changing to that directory after mounting causes it to be associated with the inode of the root of the FUSE filesystem, which then means it can't ever be unmounted because the directory is busy.

Beyond that, just a couple of small suggestions for niceties:

It would be nice to add a comment above the new users/current API call to explain why we're doing it. Otherwise it can look a lot like a noop to future readers.

Done.

In test_bogus_host, '100::' would be a better bogus host. The RFC says that traffic to 100::/64 must be immediately refused, so this way the test can't pass in case a local resolver gives an answer for 'example.null' or anything like that.

Done.

Thanks.