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.

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

Use unique name for AlertNotify tempfile #6527

Merged
merged 1 commit into from Aug 7, 2015
Merged

Use unique name for AlertNotify tempfile #6527

merged 1 commit into from Aug 7, 2015

Conversation

casey
Copy link
Contributor

@casey casey commented Aug 6, 2015

Fixes #6524

Use a unique name for alertnotify.txt tempfile during AlertNotify test

boost::filesystem::path temp = GetTempPath() / "alertnotify.txt";
boost::filesystem::remove(temp);
boost::filesystem::path temp = GetTempPath() /
boost::filesystem::unique_path("alertnotify-%%%%.txt");
Copy link
Contributor

@jonasschnelli jonasschnelli Aug 6, 2015

Choose a reason for hiding this comment

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

minor nit: wrong indent (same on L171 / not related to this PR)

Copy link
Contributor Author

@casey casey Aug 6, 2015

Choose a reason for hiding this comment

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

Isn't the right indentation four spaces? Or is it different for a line continuation?

Copy link
Contributor

@jonasschnelli jonasschnelli Aug 6, 2015

Choose a reason for hiding this comment

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

Meh. Sorry. I oversaw the line break... all good!
You can always use clang-format -i <file> if you wan't to confirm to the code-style-guidelines.

@randy-waterhouse
Copy link
Contributor

randy-waterhouse commented Aug 6, 2015

tested ACK

@jonasschnelli
Copy link
Contributor

jonasschnelli commented Aug 6, 2015

ACK.
Couldn't find any details about when files generated with boost::filesystem::unique_path are deleted.
I think adding a boost::filesystem::remove(temp); wouldn't harm?

@laanwj
Copy link
Member

laanwj commented Aug 6, 2015

ACK. Using a hardcoded/predictable temporary file name can be dangerous.

And agree with @jonasschnelli that the test should try to clean up after itself.

@casey
Copy link
Contributor Author

casey commented Aug 6, 2015

@jonasschnelli @laanwj It actually does clean up after itself; there's a call to boost::filesystem::remove(temp) at the end of the function.

@jonasschnelli
Copy link
Contributor

jonasschnelli commented Aug 6, 2015

@casey: Ah. Now i see how this works. The boost::filesystem::remove(temp); removed in this PR was there because the file was not really a random tmp file.

ACK.

@laanwj
Copy link
Member

laanwj commented Aug 7, 2015

Oh! oops. Apologies for the spurious comment, then.

@laanwj laanwj merged commit 231c560 into bitcoin:master Aug 7, 2015
laanwj added a commit that referenced this pull request Aug 7, 2015
231c560 Use unique name for AlertNotify tempfile (Casey Rodarmor)
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

make check leaves behind /tmp/alertnotify.txt
4 participants