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

qt: Backup former GUI settings on `-resetguisettings` #11338

Merged
merged 1 commit into from Sep 23, 2017

Conversation

Projects
None yet
9 participants
@laanwj
Copy link
Member

laanwj commented Sep 15, 2017

Writes the GUI settings to guisettings.bak in the data directory before wiping them. This can be used to retroactively troubleshoot issues (e.g. #11262) where -resetguisettings solves the problem.
(as discussed in yesterday's IRC meeting)

@laanwj laanwj added this to the 0.15.1 milestone Sep 15, 2017

@MeshCollider
Copy link
Member

MeshCollider left a comment

utACK 232a108, really good idea 👍

Couple of small nits

src/qt/optionsmodel.cpp Outdated
void OptionsModel::Reset()
{
QSettings settings;

// Backup old settings for troubleshooting
backupSettings(GetDataDir(true) / "guisettings.bak", settings);

This comment has been minimized.

@MeshCollider

MeshCollider Sep 15, 2017

Member

nit, GetDataDir() argument defaults to true so it'd be clearer to leave it out imo

This comment has been minimized.

@laanwj

laanwj Sep 15, 2017

Author Member

Isn't it the other way around: clearer to leave it in, because it's explicit?

This comment has been minimized.

@MeshCollider

MeshCollider Sep 15, 2017

Member

Normally I'd say yes, but in this case it just leaves me wondering what the argument is doing, because in most other cases of GetDataDir in the code, the argument is left out. Personal preference

This comment has been minimized.

@laanwj

laanwj Sep 15, 2017

Author Member

During the initialization sequence it's really important to get this right. E.g. I had to check the order in src/qt/bitcoin.cpp that calling GetDataDir(true) was allowed at this point.

Agree that boolean arguments are unclear, so are default arguments, so it's kind of choosing between evils here. Instead of an argument to GetDataDir we should have done GetSpecificDataDir and GetRootDataDir or such. Anyhow... I don't think this code is particularly unclear, I'll add 'to chain-specific datadir' in the comment.

src/qt/optionsmodel.cpp Outdated
*/
static void copySettings(QSettings &dst, const QSettings &src)
{
for (const QString &key: src.allKeys()) {

This comment has been minimized.

@MeshCollider

MeshCollider Sep 15, 2017

Member

nit, space between &key and the colon

This comment has been minimized.

@laanwj

laanwj Sep 15, 2017

Author Member

Will fix

@laanwj laanwj force-pushed the laanwj:2017_10_backup_resetguisettings branch 2 times, most recently Sep 15, 2017

@MeshCollider

This comment has been minimized.

Copy link
Member

MeshCollider commented Sep 15, 2017

re-utACK bb37a03 4e9dc85

@laanwj laanwj force-pushed the laanwj:2017_10_backup_resetguisettings branch to 4e9dc85 Sep 15, 2017

@laanwj

This comment has been minimized.

Copy link
Member Author

laanwj commented Sep 15, 2017

Travis was failing. Changed qInfo to qWarning, as qInfo was only introduced in Qt 5.5 and the result is the same anyhow: it unconditionally ends up in debug.log.

@laanwj

This comment has been minimized.

Copy link
Member Author

laanwj commented Sep 15, 2017

Added a dst.clear (suggested by @promag) to avoid the edge case of older settings staying behind.

@promag
Copy link
Member

promag left a comment

utACK modulus nits.

/** Helper function to copy contents from one QSettings to another.
* By using allKeys this also covers nested settings in a hierarchy.
*/
static void copySettings(QSettings &dst, const QSettings &src)

This comment has been minimized.

@promag

promag Sep 15, 2017

Member

Don't see the benefit of this function, move the code to backupSettings()? Otherwise & next to type name.

This comment has been minimized.

@laanwj

laanwj Sep 15, 2017

Author Member

The point is that this is a general function, it could be also e.g. used to restore QSettings from a backup, if that is ever necessary. Would prefer to keep it. Better too much factoring than too little.

This comment has been minimized.

@promag

promag Sep 15, 2017

Member

Ok then, just update to CopySettings and move &.

}

/** Back up a QSettings to an ini-formatted file. */
static void backupSettings(const fs::path &filename, const QSettings &src)

This comment has been minimized.

@promag

promag Sep 15, 2017

Member

Rename to BackupSettings and & next to type name.

*/
static void copySettings(QSettings &dst, const QSettings &src)
{
for (const QString &key : src.allKeys()) {

This comment has been minimized.

@promag

promag Sep 15, 2017

Member

& next to type name.

@@ -15,6 +15,7 @@
* wallet.dat: personal wallet (BDB) with keys and transactions
* .cookie: session RPC authentication cookie (written at start when cookie authentication is used, deleted on shutdown): since 0.12.0
* onion_private_key: cached Tor hidden service private key for `-listenonion`: since 0.12.0
* guisettings.bak: backup of former GUI settings after `-resetguisettings` is used

This comment has been minimized.

@promag

promag Sep 15, 2017

Member

.ini.bak?

This comment has been minimized.

@laanwj

laanwj Sep 15, 2017

Author Member

Yeah why not...

This comment has been minimized.

@promag

promag Sep 16, 2017

Member

Missing doc update.

@TheBlueMatt

This comment has been minimized.

Copy link
Contributor

TheBlueMatt commented Sep 15, 2017

Tested ACK f725dbb, needs squash obviously, didn't bother to try to review in-depth for whether this is going to put the guisettings.bak file in a strange datadir (bitcoin.conf-based? the old GUI settings one?) but there isnt a "correct" answer there anyway, so I cant say I care much.

@sipa

This comment has been minimized.

Copy link
Member

sipa commented Sep 15, 2017

Concept ACK

@jonasschnelli

This comment has been minimized.

Copy link
Member

jonasschnelli commented Sep 15, 2017

utACK f725dbb

Since this uses QSettings::IniFormat, there is no easy way how to restore a such backup. Is the backup file only intended to use for inspection of old values or also for a full restore? The later would very likely require a restore function (QSettings::IniFormat to native format or similar).

@laanwj

This comment has been minimized.

Copy link
Member Author

laanwj commented Sep 16, 2017

didn't bother to try to review in-depth for whether this is going to put the guisettings.bak file in a strange datadir (bitcoin.conf-based? the old GUI settings one?)

I think that's handled, it puts it in the new datadir that you have to select when using -resetguisettings.

Since this uses QSettings::IniFormat, there is no easy way how to restore a such backup. Is the backup file only intended to use for inspection of old values or also for a full restore?

Not opposed to restore functionality if there is a pressing use-case, although the intended purpose (as mentioned in OP) is diagnostics. This is much easier with .ini format. Also windows' NativeFormat cannot be exported to an arbitrary file just to other registry paths.

@promag
Copy link
Member

promag left a comment

Tested ACK. It works even if the file already exists in either INI or other format (it's always overwritten).

There are still some code nits. One small and final thing, for me this is more of a dump than backup.

@achow101

This comment has been minimized.

Copy link
Member

achow101 commented Sep 16, 2017

there is no easy way how to restore a such backup.

I don't think there needs to be a way to restore such a backup. This backup is only created when you run -resetguisettings which implies that the settings are corrupted anyways and you don't want a backup of something that's corrupted.

@promag

This comment has been minimized.

Copy link
Member

promag commented Sep 16, 2017

which implies that the settings are corrupted

Or settings are fine and there is a bug?

@achow101

This comment has been minimized.

Copy link
Member

achow101 commented Sep 16, 2017

tACK b28af38

@jonasschnelli

This comment has been minimized.

Copy link
Member

jonasschnelli commented Sep 18, 2017

Found a strange issue while testing this.
Compiling locally on OSX with MacOS 10.12.6 with Qt5.9.0 works perfect (guisettings.ini.bak is present after running with -resetguisettings).
However, using the gtitian build (https://bitcoin.jonasschnelli.ch/build/312) will result in not creating the guisettings.ini.bak file. Not sure if it is a local issue but would be good if someone could test this (via gitian)

@MarcoFalke

This comment has been minimized.

Copy link
Member

MarcoFalke commented Sep 18, 2017

@jonasschnelli The embedded commit hash does not match the advertised commit hash on the website of the downloaded gitian binaries.

@jonasschnelli

This comment has been minimized.

Copy link
Member

jonasschnelli commented Sep 19, 2017

@MarcoFalke: the commit hash is different because my build script builds it on top of master. I just did everything again and the gtitian version does not create the backup file.
https://bitcoin.jonasschnelli.ch/build/315
(Maybe someone should double-check via an gitian build).

@achow101

This comment has been minimized.

Copy link
Member

achow101 commented Sep 19, 2017

@jonasschnelli I performed my own gitian build and tested it but could not reproduce your issue. It will create guisettings.ini.bak as it should.

I did notice that if you did not have any gui settings to begin with, the file will not be created. Perhaps that's what you are seeing?

@jonasschnelli

This comment has been minimized.

Copy link
Member

jonasschnelli commented Sep 19, 2017

@achow101: even with enabling coincontrol I had no success on OSX.
What OS did you test? It may be a platform issue?

@achow101

This comment has been minimized.

Copy link
Member

achow101 commented Sep 19, 2017

@jonasschnelli I am using Ubuntu 17.04

@laanwj

This comment has been minimized.

Copy link
Member Author

laanwj commented Sep 19, 2017

I did notice that if you did not have any gui settings to begin with, the file will not be created. Perhaps that's what you are seeing?

Hm, that's interesting, haven't thought about that. I think that behavior makes sense, though.

@gmaxwell
Copy link
Member

gmaxwell left a comment

ut(not very useful at reviewing QT stuff)ACK.

qt: Backup former GUI settings on `-resetguisettings`
Writes the GUI settings to `guisettings.bak` in the data directory
before wiping them. This can be used to retroactively troubleshoot
issues (e.g. #11262) where `-resetguisettings` solves the problem.

@laanwj laanwj force-pushed the laanwj:2017_10_backup_resetguisettings branch from b28af38 to 723aa1b Sep 23, 2017

@laanwj

This comment has been minimized.

Copy link
Member Author

laanwj commented Sep 23, 2017

@laanwj laanwj merged commit 723aa1b into bitcoin:master Sep 23, 2017

1 check was pending

continuous-integration/travis-ci/pr The Travis CI build is in progress
Details

laanwj added a commit that referenced this pull request Sep 23, 2017

Merge #11338: qt: Backup former GUI settings on `-resetguisettings`
723aa1b qt: Backup former GUI settings on `-resetguisettings` (Wladimir J. van der Laan)

Pull request description:

  Writes the GUI settings to `guisettings.bak` in the data directory before wiping them. This can be used to retroactively troubleshoot issues (e.g. #11262) where `-resetguisettings` solves the problem.
  (as discussed in yesterday's IRC meeting)

Tree-SHA512: c64f5052d992eb02057ba285435f143c42d0cc456144a4c565e1c87be833737f9df750d0aee10810f85047c820d9b4f9f22fd94a6f09f4b28a9cf41b63a56586

MarcoFalke added a commit to MarcoFalke/bitcoin that referenced this pull request Oct 4, 2017

qt: Backup former GUI settings on `-resetguisettings`
Writes the GUI settings to `guisettings.bak` in the data directory
before wiping them. This can be used to retroactively troubleshoot
issues (e.g. bitcoin#11262) where `-resetguisettings` solves the problem.

Github-Pull: bitcoin#11338
Rebased-From: 723aa1b
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.