From fdd1cd8dc045db90b06497a15df7f6f232f76bda Mon Sep 17 00:00:00 2001 From: Tommi Virtanen Date: Fri, 13 Mar 2015 16:01:14 -0700 Subject: [PATCH] Remove fsrepo.At, make Open a constructor function Nobody calls At without immediately calling Open. First step, a mechanical transformation. Cleanups will follow. --- cmd/ipfs/daemon.go | 4 +- cmd/ipfs/init.go | 8 ++-- cmd/ipfs/main.go | 4 +- cmd/ipfs/tour.go | 4 +- cmd/ipfs_bootstrapd/main.go | 4 +- cmd/ipfs_routingd/main.go | 4 +- cmd/ipfswatch/main.go | 4 +- core/commands/bootstrap.go | 12 ++--- core/commands/config.go | 9 ++-- repo/fsrepo/fsrepo.go | 85 +++++++++++++++++------------------ repo/fsrepo/fsrepo_test.go | 43 +++++++----------- test/supernode_client/main.go | 4 +- updates/updates.go | 4 +- 13 files changed, 87 insertions(+), 102 deletions(-) diff --git a/cmd/ipfs/daemon.go b/cmd/ipfs/daemon.go index e8794f68a..8589542e4 100644 --- a/cmd/ipfs/daemon.go +++ b/cmd/ipfs/daemon.go @@ -118,8 +118,8 @@ func daemonFunc(req cmds.Request, res cmds.Response) { // acquire the repo lock _before_ constructing a node. we need to make // sure we are permitted to access the resources (datastore, etc.) - repo := fsrepo.At(req.Context().ConfigRoot) - if err := repo.Open(); err != nil { + repo, err := fsrepo.Open(req.Context().ConfigRoot) + if err != nil { res.SetError(debugerror.Errorf("Couldn't obtain lock. Is another daemon already running?"), cmds.ErrNormal) return } diff --git a/cmd/ipfs/init.go b/cmd/ipfs/init.go index 72446cf3a..f962792da 100644 --- a/cmd/ipfs/init.go +++ b/cmd/ipfs/init.go @@ -110,8 +110,8 @@ func addDefaultAssets(out io.Writer, repoRoot string) error { ctx, cancel := context.WithCancel(context.Background()) defer cancel() - r := fsrepo.At(repoRoot) - if err := r.Open(); err != nil { // NB: repo is owned by the node + r, err := fsrepo.Open(repoRoot) + if err != nil { // NB: repo is owned by the node return err } @@ -163,8 +163,8 @@ func initializeIpnsKeyspace(repoRoot string) error { ctx, cancel := context.WithCancel(context.Background()) defer cancel() - r := fsrepo.At(repoRoot) - if err := r.Open(); err != nil { // NB: repo is owned by the node + r, err := fsrepo.Open(repoRoot) + if err != nil { // NB: repo is owned by the node return err } diff --git a/cmd/ipfs/main.go b/cmd/ipfs/main.go index 2bca9dd45..abe2a6594 100644 --- a/cmd/ipfs/main.go +++ b/cmd/ipfs/main.go @@ -193,8 +193,8 @@ func (i *cmdInvocation) constructNodeFunc(ctx context.Context) func() (*core.Ipf return nil, errors.New("constructing node without a request context") } - r := fsrepo.At(i.req.Context().ConfigRoot) - if err := r.Open(); err != nil { // repo is owned by the node + r, err := fsrepo.Open(i.req.Context().ConfigRoot) + if err != nil { // repo is owned by the node return nil, err } diff --git a/cmd/ipfs/tour.go b/cmd/ipfs/tour.go index 9a5277301..f2aae417d 100644 --- a/cmd/ipfs/tour.go +++ b/cmd/ipfs/tour.go @@ -193,8 +193,8 @@ func tourGet(id tour.ID) (*tour.Topic, error) { // TODO share func func writeConfig(path string, cfg *config.Config) error { // NB: This needs to run on the daemon. - r := fsrepo.At(path) - if err := r.Open(); err != nil { + r, err := fsrepo.Open(path) + if err != nil { return err } defer r.Close() diff --git a/cmd/ipfs_bootstrapd/main.go b/cmd/ipfs_bootstrapd/main.go index a215d65b0..699d569b0 100644 --- a/cmd/ipfs_bootstrapd/main.go +++ b/cmd/ipfs_bootstrapd/main.go @@ -57,8 +57,8 @@ func run() error { } } - repo := fsrepo.At(repoPath) - if err := repo.Open(); err != nil { // owned by node + repo, err := fsrepo.Open(repoPath) + if err != nil { // owned by node return err } diff --git a/cmd/ipfs_routingd/main.go b/cmd/ipfs_routingd/main.go index d02a9744a..7cd35a330 100644 --- a/cmd/ipfs_routingd/main.go +++ b/cmd/ipfs_routingd/main.go @@ -57,8 +57,8 @@ func run() error { return err } } - repo := fsrepo.At(repoPath) - if err := repo.Open(); err != nil { // owned by node + repo, err := fsrepo.Open(repoPath) + if err != nil { // owned by node return err } diff --git a/cmd/ipfswatch/main.go b/cmd/ipfswatch/main.go index 7aff99a3f..e9e397ed2 100644 --- a/cmd/ipfswatch/main.go +++ b/cmd/ipfswatch/main.go @@ -65,8 +65,8 @@ func run(ipfsPath, watchPath string) error { return err } - r := fsrepo.At(ipfsPath) - if err := r.Open(); err != nil { + r, err := fsrepo.Open(ipfsPath) + if err != nil { // TODO handle case: daemon running // TODO handle case: repo doesn't exist or isn't initialized return err diff --git a/core/commands/bootstrap.go b/core/commands/bootstrap.go index 4c1d0d42d..c2116a555 100644 --- a/core/commands/bootstrap.go +++ b/core/commands/bootstrap.go @@ -66,8 +66,8 @@ in the bootstrap list). return } - r := fsrepo.At(req.Context().ConfigRoot) - if err := r.Open(); err != nil { + r, err := fsrepo.Open(req.Context().ConfigRoot) + if err != nil { res.SetError(err, cmds.ErrNormal) return } @@ -143,8 +143,8 @@ var bootstrapRemoveCmd = &cmds.Command{ return } - r := fsrepo.At(req.Context().ConfigRoot) - if err := r.Open(); err != nil { + r, err := fsrepo.Open(req.Context().ConfigRoot) + if err != nil { res.SetError(err, cmds.ErrNormal) return } @@ -192,8 +192,8 @@ var bootstrapListCmd = &cmds.Command{ }, Run: func(req cmds.Request, res cmds.Response) { - r := fsrepo.At(req.Context().ConfigRoot) - if err := r.Open(); err != nil { + r, err := fsrepo.Open(req.Context().ConfigRoot) + if err != nil { res.SetError(err, cmds.ErrNormal) return } diff --git a/core/commands/config.go b/core/commands/config.go index e0d8d586e..e57b6626e 100644 --- a/core/commands/config.go +++ b/core/commands/config.go @@ -64,14 +64,13 @@ Set the value of the 'datastore.path' key: args := req.Arguments() key := args[0] - r := fsrepo.At(req.Context().ConfigRoot) - if err := r.Open(); err != nil { + r, err := fsrepo.Open(req.Context().ConfigRoot) + if err != nil { res.SetError(err, cmds.ErrNormal) return } defer r.Close() - var err error var output *ConfigField if len(args) == 2 { value := args[1] @@ -182,8 +181,8 @@ can't be undone. cmds.FileArg("file", true, false, "The file to use as the new config"), }, Run: func(req cmds.Request, res cmds.Response) { - r := fsrepo.At(req.Context().ConfigRoot) - if err := r.Open(); err != nil { + r, err := fsrepo.Open(req.Context().ConfigRoot) + if err != nil { res.SetError(err, cmds.ErrNormal) return } diff --git a/repo/fsrepo/fsrepo.go b/repo/fsrepo/fsrepo.go index ab56fa7f1..626ecb18d 100644 --- a/repo/fsrepo/fsrepo.go +++ b/repo/fsrepo/fsrepo.go @@ -75,13 +75,51 @@ type FSRepo struct { var _ repo.Repo = (*FSRepo)(nil) -// At returns a handle to an FSRepo at the provided |path|. -func At(repoPath string) *FSRepo { - // This method must not have side-effects. - return &FSRepo{ +// Open the FSRepo at path. Returns an error if the repo is not +// initialized. +func Open(repoPath string) (*FSRepo, error) { + packageLock.Lock() + defer packageLock.Unlock() + + r := &FSRepo{ path: path.Clean(repoPath), state: unopened, // explicitly set for clarity } + + expPath, err := u.TildeExpansion(r.path) + if err != nil { + return nil, err + } + r.path = expPath + + if r.state != unopened { + return nil, debugerror.Errorf("repo is %s", r.state) + } + if !isInitializedUnsynced(r.path) { + return nil, debugerror.New("ipfs not initialized, please run 'ipfs init'") + } + // check repo path, then check all constituent parts. + // TODO acquire repo lock + // TODO if err := initCheckDir(logpath); err != nil { // } + if err := dir.Writable(r.path); err != nil { + return nil, err + } + + if err := r.openConfig(); err != nil { + return nil, err + } + + if err := r.openDatastore(); err != nil { + return nil, err + } + + // log.Debugf("writing eventlogs to ...", c.path) + configureEventLoggerAtRepoPath(r.config, r.path) + + if err := r.transitionToOpened(); err != nil { + return nil, err + } + return r, nil } // ConfigAt returns an error if the FSRepo at the given path is not @@ -236,45 +274,6 @@ func configureEventLoggerAtRepoPath(c *config.Config, repoPath string) { eventlog.Configure(eventlog.OutputRotatingLogFile(rotateConf)) } -// Open returns an error if the repo is not initialized. -func (r *FSRepo) Open() error { - - packageLock.Lock() - defer packageLock.Unlock() - - expPath, err := u.TildeExpansion(r.path) - if err != nil { - return err - } - r.path = expPath - - if r.state != unopened { - return debugerror.Errorf("repo is %s", r.state) - } - if !isInitializedUnsynced(r.path) { - return debugerror.New("ipfs not initialized, please run 'ipfs init'") - } - // check repo path, then check all constituent parts. - // TODO acquire repo lock - // TODO if err := initCheckDir(logpath); err != nil { // } - if err := dir.Writable(r.path); err != nil { - return err - } - - if err := r.openConfig(); err != nil { - return err - } - - if err := r.openDatastore(); err != nil { - return err - } - - // log.Debugf("writing eventlogs to ...", c.path) - configureEventLoggerAtRepoPath(r.config, r.path) - - return r.transitionToOpened() -} - func (r *FSRepo) closeDatastore() error { dsLock.Lock() defer dsLock.Unlock() diff --git a/repo/fsrepo/fsrepo_test.go b/repo/fsrepo/fsrepo_test.go index 06e3ba623..a526a31c9 100644 --- a/repo/fsrepo/fsrepo_test.go +++ b/repo/fsrepo/fsrepo_test.go @@ -33,19 +33,6 @@ func TestRemove(t *testing.T) { assert.Nil(Remove(path), t, "can remove a repository") } -func TestCannotBeReopened(t *testing.T) { - t.Parallel() - path := testRepoPath("", t) - assert.Nil(Init(path, &config.Config{}), t) - r := At(path) - assert.Nil(r.Open(), t) - assert.Nil(r.Close(), t) - assert.Err(r.Open(), t, "shouldn't be possible to re-open the repo") - - // mutable state is the enemy. Take Close() as an opportunity to reduce - // entropy. Callers ought to start fresh with a new handle by calling `At`. -} - func TestCanManageReposIndependently(t *testing.T) { t.Parallel() pathA := testRepoPath("a", t) @@ -60,10 +47,10 @@ func TestCanManageReposIndependently(t *testing.T) { assert.True(IsInitialized(pathB), t, "b should be initialized") t.Log("open the two repos") - repoA := At(pathA) - repoB := At(pathB) - assert.Nil(repoA.Open(), t, "a") - assert.Nil(repoB.Open(), t, "b") + repoA, err := Open(pathA) + assert.Nil(err, t, "a") + repoB, err := Open(pathB) + assert.Nil(err, t, "b") t.Log("close and remove b while a is open") assert.Nil(repoB.Close(), t, "close b") @@ -80,15 +67,15 @@ func TestDatastoreGetNotAllowedAfterClose(t *testing.T) { assert.True(!IsInitialized(path), t, "should NOT be initialized") assert.Nil(Init(path, &config.Config{}), t, "should initialize successfully") - r := At(path) - assert.Nil(r.Open(), t, "should open successfully") + r, err := Open(path) + assert.Nil(err, t, "should open successfully") k := "key" data := []byte(k) assert.Nil(r.Datastore().Put(datastore.NewKey(k), data), t, "Put should be successful") assert.Nil(r.Close(), t) - _, err := r.Datastore().Get(datastore.NewKey(k)) + _, err = r.Datastore().Get(datastore.NewKey(k)) assert.Err(err, t, "after closer, Get should be fail") } @@ -97,16 +84,16 @@ func TestDatastorePersistsFromRepoToRepo(t *testing.T) { path := testRepoPath("test", t) assert.Nil(Init(path, &config.Config{}), t) - r1 := At(path) - assert.Nil(r1.Open(), t) + r1, err := Open(path) + assert.Nil(err, t) k := "key" expected := []byte(k) assert.Nil(r1.Datastore().Put(datastore.NewKey(k), expected), t, "using first repo, Put should be successful") assert.Nil(r1.Close(), t) - r2 := At(path) - assert.Nil(r2.Open(), t) + r2, err := Open(path) + assert.Nil(err, t) v, err := r2.Datastore().Get(datastore.NewKey(k)) assert.Nil(err, t, "using second repo, Get should be successful") actual, ok := v.([]byte) @@ -120,10 +107,10 @@ func TestOpenMoreThanOnceInSameProcess(t *testing.T) { path := testRepoPath("", t) assert.Nil(Init(path, &config.Config{}), t) - r1 := At(path) - r2 := At(path) - assert.Nil(r1.Open(), t, "first repo should open successfully") - assert.Nil(r2.Open(), t, "second repo should open successfully") + r1, err := Open(path) + assert.Nil(err, t, "first repo should open successfully") + r2, err := Open(path) + assert.Nil(err, t, "second repo should open successfully") assert.True(r1.ds == r2.ds, t, "repos should share the datastore") assert.Nil(r1.Close(), t) diff --git a/test/supernode_client/main.go b/test/supernode_client/main.go index 13f39ac8b..844355516 100644 --- a/test/supernode_client/main.go +++ b/test/supernode_client/main.go @@ -66,8 +66,8 @@ func run() error { repoPath := gopath.Join(cwd, ".go-ipfs") if err := ensureRepoInitialized(repoPath); err != nil { } - repo := fsrepo.At(repoPath) - if err := repo.Open(); err != nil { // owned by node + repo, err := fsrepo.Open(repoPath) + if err != nil { // owned by node return err } cfg := repo.Config() diff --git a/updates/updates.go b/updates/updates.go index 229ba95d5..81f1eb444 100644 --- a/updates/updates.go +++ b/updates/updates.go @@ -212,8 +212,8 @@ func CliCheckForUpdates(cfg *config.Config, repoPath string) error { // if we checked successfully. if err == ErrNoUpdateAvailable { log.Noticef("No update available, checked on %s", time.Now()) - r := fsrepo.At(repoPath) - if err := r.Open(); err != nil { + r, err := fsrepo.Open(repoPath) + if err != nil { return err } if err := recordUpdateCheck(cfg); err != nil {