Skip to content

remove pathAddr field from CAddrDB class#6308

Closed
Diapolo wants to merge 1 commit intobitcoin:masterfrom
Diapolo:addrdb
Closed

remove pathAddr field from CAddrDB class#6308
Diapolo wants to merge 1 commit intobitcoin:masterfrom
Diapolo:addrdb

Conversation

@Diapolo
Copy link
Copy Markdown

@Diapolo Diapolo commented Jun 19, 2015

  • removes the need for a boost::filesystem::path field, which caused
    issues on shutdown because of a static initialized internal pointer

See #6282, which is why I choose to open this pull.

- removes the need for a boost::filesystem::path field, which caused
  issues on shutdown because of a static initialized internal pointer
@jonasschnelli
Copy link
Copy Markdown
Contributor

Magic! I just was writing a CBanDB (ban list disk store) and where thinking about the same change for CAddrDB. But i would have done it differently.
Why not keeping the constructor and creating the temp file by adding the random number to pathAddr?
After your PR we would have multiples of "peers.dat".

@laanwj
Copy link
Copy Markdown
Member

laanwj commented Jun 19, 2015

This is only relevant for structures defined at a global level. AddrDB isn't.

Are you sure this is causing a problem? Are you still experiencing crashes after #6282?

IMO we should be switching to controlled lifetime for such global db objects, instead, That's a less fragile solution than randomly nuking usages of boost::path in classes in the oft case they give problems at the end of the program.

@laanwj
Copy link
Copy Markdown
Member

laanwj commented Jun 19, 2015

There is no need for this change, closing.

@laanwj laanwj closed this Jun 19, 2015
@Diapolo
Copy link
Copy Markdown
Author

Diapolo commented Jun 19, 2015

I thought it would be nice to not need to store the path ;)... well, I'm fine with getting it rejected.

@Diapolo Diapolo deleted the addrdb branch June 19, 2015 19:50
@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.

3 participants