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.

Sign up for GitHub

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

Jump to bottom

write api file automically #7282

Merged
merged 3 commits into from
May 10, 2020
Merged

write api file automically #7282

Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
17ec104
write api file automically
poonai May 6, 2020
03b1a60
change tmp name
poonai May 7, 2020
0a17fc8
clean up tmp file
poonai May 10, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
16 changes: 12 additions & 4 deletions repo/fsrepo/fsrepo.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -359,14 +359,22 @@ func (r *FSRepo) Path() string {

// SetAPIAddr writes the API Addr to the /api file.
func (r *FSRepo) SetAPIAddr(addr ma.Multiaddr) error {
Copy link
Member

Choose a reason for hiding this comment

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

If we fail to rename the temporary file, let's remove it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I would like to keep as it is. It feels like too much defensive programming.

When users run the program next time, newly created tmp will override the existing tmp file.

If you like to remove, I'll add code to remove the tmp file if rename fails

Copy link
Member

Choose a reason for hiding this comment

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

Well, it is defensive programming. While this is unlikely to fail on Linux, renaming can fail on Windows if some other user is accessing the API file at the same time. Yes, we'll end up deleting the temporary file later, but that isn't a reason to not at least try to clean it up early.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Made changes. Thanks for the explaining the reason.

f, err := os.Create(filepath.Join(r.path, apiFile))
// Create a temp file to write the address, so that we don't leave empty file when the
// program crashes after creating the file.
f, err := os.Create(filepath.Join(r.path, apiFile+"-tmp"))
Copy link
Member

Choose a reason for hiding this comment

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

I'd name this something like:

Suggested change
f, err := os.Create(filepath.Join(r.path, apiFile+"-tmp"))
f, err := os.Create(filepath.Join(r.path, "." + apiFile+".tmp"))

if err != nil {
return err
}
defer f.Close()

_, err = f.WriteString(addr.String())
return err
if _, err = f.WriteString(addr.String()); err != nil {
return err
}
if err = f.Close(); err != nil {
return err
}

// Atomically rename the temp file to the correct file name.
return os.Rename(filepath.Join(r.path, apiFile+"-tmp"), filepath.Join(r.path, apiFile))
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
return os.Rename(filepath.Join(r.path, apiFile+"-tmp"), filepath.Join(r.path, apiFile))
return os.Rename(filepath.Join(r.path, "." + apiFile+".tmp"), filepath.Join(r.path, apiFile))

}

// openConfig returns an error if the config file is not present.
Expand Down