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

pull laanwj wants to merge 1 commits into bitcoin:master from laanwj:2017_10_backup_resetguisettings changing 2 files +23 −0
  1. laanwj commented at 9:57 AM on September 15, 2017: member

    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)

  2. laanwj added the label GUI on Sep 15, 2017
  3. laanwj added the label Needs backport on Sep 15, 2017
  4. laanwj added this to the milestone 0.15.1 on Sep 15, 2017
  5. in src/qt/optionsmodel.cpp:177 in 232a1089dd outdated 
     172 |  void OptionsModel::Reset()
 173 |  {
 174 |      QSettings settings;
 175 |  
 176 | +    // Backup old settings for troubleshooting
 177 | +    backupSettings(GetDataDir(true) / "guisettings.bak", settings);
    meshcollider commented at 10:00 AM on September 15, 2017:

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

    laanwj commented at 10:04 AM on September 15, 2017:

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

    meshcollider commented at 10:08 AM on September 15, 2017:

    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

    laanwj commented at 10:11 AM on September 15, 2017:

    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.

  6. in src/qt/optionsmodel.cpp:159 in 232a1089dd outdated 
     150 | @@ -151,10 +151,31 @@ void OptionsModel::Init(bool resetSettings)
 151 |      language = settings.value("language").toString();
 152 |  }
 153 |  
 154 | +/** Helper function to copy contents from one QSettings to another.
 155 | + * By using allKeys this also covers nested settings in a hierarchy.
 156 | + */
 157 | +static void copySettings(QSettings &dst, const QSettings &src)
 158 | +{
 159 | +    for (const QString &key: src.allKeys()) {
    meshcollider commented at 10:01 AM on September 15, 2017:

    nit, space between &key and the colon

    laanwj commented at 10:04 AM on September 15, 2017:

    Will fix

  7. meshcollider commented at 10:02 AM on September 15, 2017: contributor

    utACK https://github.com/bitcoin/bitcoin/pull/11338/commits/232a1089dd90878ca95a01f0ce053c4b1ffba88e, really good idea :+1:

    Couple of small nits

  8. laanwj force-pushed on Sep 15, 2017
  9. laanwj force-pushed on Sep 15, 2017
  11. laanwj force-pushed on Sep 15, 2017
  12. laanwj commented at 10:56 AM on September 15, 2017: member

    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.

  13. laanwj commented at 1:17 PM on September 15, 2017: member

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

  14. in src/qt/optionsmodel.cpp:157 in f725dbbfc2 outdated 
     150 | @@ -151,10 +151,32 @@ void OptionsModel::Init(bool resetSettings)
 151 |      language = settings.value("language").toString();
 152 |  }
 153 |  
 154 | +/** Helper function to copy contents from one QSettings to another.
 155 | + * By using allKeys this also covers nested settings in a hierarchy.
 156 | + */
 157 | +static void copySettings(QSettings &dst, const QSettings &src)
    promag commented at 1:31 PM on September 15, 2017:

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

    laanwj commented at 2:17 PM on September 15, 2017:

    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.

    promag commented at 2:23 PM on September 15, 2017:

    Ok then, just update to CopySettings and move &.

  15. in src/qt/optionsmodel.cpp:165 in f725dbbfc2 outdated 
     160 | +        dst.setValue(key, src.value(key));
 161 | +    }
 162 | +}
 163 | +
 164 | +/** Back up a QSettings to an ini-formatted file. */
 165 | +static void backupSettings(const fs::path &filename, const QSettings &src)
    promag commented at 1:32 PM on September 15, 2017:

    Rename to BackupSettings and & next to type name.

  16. in src/qt/optionsmodel.cpp:159 in f725dbbfc2 outdated 
     150 | @@ -151,10 +151,32 @@ void OptionsModel::Init(bool resetSettings)
 151 |      language = settings.value("language").toString();
 152 |  }
 153 |  
 154 | +/** Helper function to copy contents from one QSettings to another.
 155 | + * By using allKeys this also covers nested settings in a hierarchy.
 156 | + */
 157 | +static void copySettings(QSettings &dst, const QSettings &src)
 158 | +{
 159 | +    for (const QString &key : src.allKeys()) {
    promag commented at 1:33 PM on September 15, 2017:

    & next to type name.

  17. in doc/files.md:18 in f725dbbfc2 outdated 
      14 | @@ -15,6 +15,7 @@
  15 |  * wallet.dat: personal wallet (BDB) with keys and transactions
  16 |  * .cookie: session RPC authentication cookie (written at start when cookie authentication is used, deleted on shutdown): since 0.12.0
  17 |  * onion_private_key: cached Tor hidden service private key for `-listenonion`: since 0.12.0
  18 | +* guisettings.bak: backup of former GUI settings after `-resetguisettings` is used
    promag commented at 1:35 PM on September 15, 2017:

    .ini.bak?

    laanwj commented at 2:19 PM on September 15, 2017:

    Yeah why not...

    promag commented at 2:17 PM on September 16, 2017:

    Missing doc update.

  18. promag commented at 1:37 PM on September 15, 2017: member

    utACK modulus nits.

  19. TheBlueMatt commented at 9:18 PM on September 15, 2017: member

    Tested ACK f725dbbfc2b096dabb53f9ee9b4f5ae3d659b412, 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.

  20. sipa commented at 11:26 PM on September 15, 2017: member

    Concept ACK

  21. jonasschnelli commented at 11:30 PM on September 15, 2017: contributor

    utACK f725dbbfc2b096dabb53f9ee9b4f5ae3d659b412

    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).

  22. laanwj commented at 5:04 AM on September 16, 2017: member

    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.

  23. promag commented at 2:25 PM on September 16, 2017: member

    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.

  24. achow101 commented at 4:19 PM on September 16, 2017: member

    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.

  25. promag commented at 4:22 PM on September 16, 2017: member

    which implies that the settings are corrupted

    Or settings are fine and there is a bug?

  26. achow101 commented at 4:43 PM on September 16, 2017: member

    tACK b28af386a38e6cb636683346b671b90d44382d74

  27. jonasschnelli commented at 5:07 AM on September 18, 2017: contributor

    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)

  28. MarcoFalke commented at 10:06 AM on September 18, 2017: member

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

  29. jonasschnelli commented at 3:57 AM on September 19, 2017: contributor

    @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).

  30. achow101 commented at 4:54 AM on September 19, 2017: member

    @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?

  31. jonasschnelli commented at 2:08 PM on September 19, 2017: contributor

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

  32. achow101 commented at 2:19 PM on September 19, 2017: member

    @jonasschnelli I am using Ubuntu 17.04

  33. laanwj commented at 2:29 PM on September 19, 2017: member

    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.

  34. gmaxwell approved
  35. gmaxwell commented at 4:47 AM on September 23, 2017: contributor

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

  36. 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.
    723aa1b875
  37. laanwj force-pushed on Sep 23, 2017
  38. laanwj commented at 7:39 AM on September 23, 2017: member

    Squashed 4e9dc85e0f0ef09a8c123681fa1758f1b173c9d7 f725dbbfc2b096dabb53f9ee9b4f5ae3d659b412 0f29983c80f91fccddfff7e045ed224ce93bce81 b28af386a38e6cb636683346b671b90d44382d74 → 723aa1b8752c1d6c6c0a76059c532ebe2f406fc1

  39. laanwj merged this on Sep 23, 2017
  40. laanwj closed this on Sep 23, 2017
  41. laanwj referenced this in commit 10a20bf770 on Sep 23, 2017
  42. MarcoFalke referenced this in commit 6a62c745a9 on Oct 4, 2017
  43. MarcoFalke removed the label Needs backport on Oct 4, 2017
  44. jasonbcox referenced this in commit cbabd40c5b on Sep 13, 2019
  45. codablock referenced this in commit 6ab6809598 on Sep 25, 2019
  46. jonspock referenced this in commit 084b67338d on Dec 22, 2019
  47. proteanx referenced this in commit 30818584ed on Dec 23, 2019
  48. barrystyle referenced this in commit acb1fa50d3 on Jan 22, 2020
  49. MarcoFalke locked this on Sep 8, 2021
Contributors
laanwjmeshcolliderpromagTheBlueMattsipajonasschnelliachow101MarcoFalkegmaxwell
Labels
GUI

Milestone
0.15.2

Linked (view graph)
#11262 bitcoin-qt segmentation fault
github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2026-04-13 15:15 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me