search

CircuitHappy / missinglink

Missing Link binary meant to run on embedded platforms like Raspberry Pi
GNU General Public License v2.0
7 stars 0 forks source link

A mis-matched config setting will cause settings.load() to crash ML #14

Open vizzie opened 6 years ago

vizzie commented 6 years ago

If libconfig expects a setting to be of a certain type and it is not that type, ML terminates.

vizzie commented 6 years ago

We probably need to add some kind of safeguard... maybe the setting that is getting loaded gets set to its default.

vizzie commented 6 years ago

Oct 06 15:50:15 MissingLink-ce87 systemd[1]: Started Missing Link. Oct 06 15:50:15 MissingLink-ce87 missing_link[8041]: Failed to read config file Oct 06 15:50:15 MissingLink-ce87 missing_link[8041]: terminate called after throwing an instance of 'libconfig::SettingTypeException' Oct 06 15:50:15 MissingLink-ce87 missing_link[8041]: what(): SettingTypeException Oct 06 15:50:15 MissingLink-ce87 systemd[1]: missing-link.service: Main process exited, code=killed, status=6/ABRT Oct 06 15:50:16 MissingLink-ce87 systemd[1]: missing-link.service: Unit entered failed state. Oct 06 15:50:16 MissingLink-ce87 systemd[1]: missing-link.service: Failed with result 'signal'.

ndonald2 commented 6 years ago

Good catch! I agree that it will be important to handle this exception, but there are a few things to keep in mind.

  1. Since this is related to the sync mode Int/Boolean refactor, which deals with a completely new setting, actual users shouldn't experience this in the wild. On a clean upgrade it should just fail to read the setting from the file (I think) since it's a brand new setting. This is happening to you since your settings file already has the sync mode setting as an Int and you're trying to read it as a Boolean. If we ever needed to deploy an update that changed the schema of the settings file like that, we'd need to make sure there's a clean upgrade path - check settings file version, read old version of file, write new version with new settings, update settings file version. Hopefully we can put that off for awhile.

  2. We probably need to add some kind of safeguard... maybe the setting that is getting loaded gets set to its default.

I'll have to look at how libconfig defines SettingTypeException, but I think a simpler solution would be to just blow away the settings file in that case. It sucks that the user would lose their previous settings, but see point 1 above - this is very unlikely to happen in the wild as long as we are careful about clean upgrades. It would be simpler to wrap the whole setting loading routine in a try/catch and handle it by blowing the file away and starting from scratch than to wrap each setting load call and handle all possible exceptions separately.

vizzie commented 6 years ago

update on this: the failure was because I forgot to change start_stop_sync to a bool in the Settings struct. d'oh! So I think we are good. It wasn't failing at the actual loading from the text file, but at the spot where it was trying to assign it to the settings.start_stop_sync

So I think we are good here, and your above points make sense. I'm gonna close this issue unless you think there's anything worth exploring first.