-
Notifications
You must be signed in to change notification settings - Fork 20k
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
swarm/api: support mounting manifests via FUSE #3690
Conversation
@jmozah, thanks for your PR! By analyzing the history of the files in this pull request, we identified @zsfelfoldi, @fjl and @karalabe to be potential reviewers. |
This also supports random seek on a big file making full use of the lazyreader in swarm. |
reviewing this now |
Thank you for your contribution! Your commits seem to not adhere to the repository coding standards
Please check the contribution guidelines for more details. This message was auto-generated by https://gitcop.com |
Thank you for your contribution! Your commits seem to not adhere to the repository coding standards
Please check the contribution guidelines for more details. This message was auto-generated by https://gitcop.com |
1 similar comment
Thank you for your contribution! Your commits seem to not adhere to the repository coding standards
Please check the contribution guidelines for more details. This message was auto-generated by https://gitcop.com |
|
@zelig Need some fixes based on OS (compilation is failing in windows as Fuse is not supported in windows). Will fix that and rebase it. |
Thank you for your contribution! Your commits seem to not adhere to the repository coding standards
Please check the contribution guidelines for more details. This message was auto-generated by https://gitcop.com |
1 similar comment
Thank you for your contribution! Your commits seem to not adhere to the repository coding standards
Please check the contribution guidelines for more details. This message was auto-generated by https://gitcop.com |
Thank you for your contribution! Your commits seem to not adhere to the repository coding standards
Please check the contribution guidelines for more details. This message was auto-generated by https://gitcop.com |
Thank you for your contribution! Your commits seem to not adhere to the repository coding standards
Please check the contribution guidelines for more details. This message was auto-generated by https://gitcop.com |
internal/web3ext/web3ext.go
Outdated
} | ||
|
||
const SWARMFS_JS = ` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please keep the list is this file sorted alphabetically. Both the above and below too (swarm is notorious for adding stuff all over the place in this file :P)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sure.. will do that
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
Thank you for your contribution! Your commits seem to not adhere to the repository coding standards
Please check the contribution guidelines for more details. This message was auto-generated by https://gitcop.com |
Thank you for your contribution! Your commits seem to not adhere to the repository coding standards
Please check the contribution guidelines for more details. This message was auto-generated by https://gitcop.com |
Thank you for your contribution! Your commits seem to not adhere to the repository coding standards
Please check the contribution guidelines for more details. This message was auto-generated by https://gitcop.com |
Thank you for your contribution! Your commits seem to not adhere to the repository coding standards
Please check the contribution guidelines for more details. This message was auto-generated by https://gitcop.com |
Thank you for your contribution! Your commits seem to not adhere to the repository coding standards
Please check the contribution guidelines for more details. This message was auto-generated by https://gitcop.com |
Thank you for your contribution! Your commits seem to not adhere to the repository coding standards
Please check the contribution guidelines for more details. This message was auto-generated by https://gitcop.com |
Thank you for your contribution! Your commits seem to not adhere to the repository coding standards
Please check the contribution guidelines for more details. This message was auto-generated by https://gitcop.com |
Thank you for your contribution! Your commits seem to not adhere to the repository coding standards
Please check the contribution guidelines for more details. This message was auto-generated by https://gitcop.com |
Thank you for your contribution! Your commits seem to not adhere to the repository coding standards
Please check the contribution guidelines for more details. This message was auto-generated by https://gitcop.com |
Thank you for your contribution! Your commits seem to not adhere to the repository coding standards
Please check the contribution guidelines for more details. This message was auto-generated by https://gitcop.com |
Thank you for your contribution! Your commits seem to not adhere to the repository coding standards
Please check the contribution guidelines for more details. This message was auto-generated by https://gitcop.com |
Thank you for your contribution! Your commits seem to not adhere to the repository coding standards
Please check the contribution guidelines for more details. This message was auto-generated by https://gitcop.com |
Thank you for your contribution! Your commits seem to not adhere to the repository coding standards
Please check the contribution guidelines for more details. This message was auto-generated by https://gitcop.com |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
swarm/api/swarmfs_unix.go
Outdated
|
||
|
||
|
||
func (self *SwarmFS) Mount(mhash, mountpoint string) bool { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
RPC supports (interface{}, err) composite return values. I think Mount and Unmount should use it, for proper RPC error responses
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah.. will fix this
@@ -216,7 +220,7 @@ func (self *Swarm) Stop() error { | |||
if self.lstore != nil { | |||
self.lstore.DbStore.Close() | |||
} | |||
|
|||
self.sfs.Stop() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can something foul happen if we do not unmount active mounts. E.g. KILLSIG?
SIGINT and recently also SIGTERM are caught gracefully
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the device is busy.. we cannot unmount and this will return after 5 seconds without unmounting. We cannot do much here... the mount will be visible from "mount" command and it can be unmounted using external "umount" command ....
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we can always unmount from the bash prompt "umount swarmfs"
swarm/swarm.go
Outdated
Namespace: "swarmfs", | ||
Version: api.Swarmfs_Version, | ||
Service: api.NewSwarmFS(self.api), | ||
Public: true, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
well I think the idea was that we do not 'public' an API that can read/write the filesystem.
Not sure if there is rpcapis
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
aah.. ok.. i will change it to false...
also the travis issue with sudo? is that gonna be fixed? @karalabe |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please fix the (minor) issues. I still need to actually try it out but the code looks OK.
General feedback: The fuse stuff should probably be in its own package e.g. swarm/fusefs.
swarm/api/swarmfs.go
Outdated
}) | ||
return swarmfs | ||
|
||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On any but the first call, NewSwarmFS returns the same SwarmFS regardless of api. IMHO that's confusing and might lead to bugs later. Why does it need to be a global singleton?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i thought about this... let me see if there is clean way to avoid having singleton logic.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried moving the fuse stuff inside a package.. but this right now depends on some methods which are not exposed as api... let me attempt this once more ..
swarm/api/swarmfs_unix.go
Outdated
@@ -0,0 +1,266 @@ | |||
// +build !windows |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please put build constraints below the license header.
// Copyright...
// +build !windows
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
aah okay.. will fix it
build/ci.go
Outdated
packages = append(packages, strings.TrimSpace(line)) | ||
} | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMHO it would be better to remove anything with github.com/ethereum/go-ethereum/vendor/ prefix, on all platforms.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It cannot be removed for all platforms, as some fuse test are dependent on it.
It is removed for windows since windows does not support fuse or user level file system support in general.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should not remove for all platforms since "vendor/basil.org/fuse" is required by other platforms for the test cases to work.
Fuse support is not there only in windows.. thats why i removed those for windows.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If I understand correctly, this change fixes make all
and related commands by excluding "bazil.org/fuse" from the expansion of "./..." for the purpose of building and testing.
My suggestion is that all vendored packages should be excluded. Building vendored packages explicitly is almost never needed; testing them doesn't do anything because we strip their _test.go files when adding them. They will still be built as dependencies of non-vendored packages.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I gotta say that it sucks that bazil.org/fuse doesn't even build on Windows.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
aah... i didnt see this recent comment...
Are you saying that i remove compiling vendor packages for all platforms...?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed it.. i removed all vendors packages
Minor notes about the commit message (you don't need to fix this, I'll edit it when squashing).
|
And a question: are you planning to move fuse support to swarm/fusefs or not? |
@fjl Not in this release... That will require some refactoring of the code in swarm/api and expose some methods which are not exposed for now. |
Cool, thanks. I'm fine with keeping it in swarm/api for now. |
@fjl no issues.. i will resolve the conflict and rebase |
@fjl fixed the vendor skipping.. |
This PR adds mounting swarm hash/ensname as a fuse fie system.
It adds new commands to swarm cmdline
Tested in linux and osx.