This patch adds a check for invalid config values and set them to a default value in case they're invalid
Details
Details
- Reviewers
vladislavbelov - Group Reviewers
Restricted Owners Package (Owns No Changed Paths)
Try to set invalid values in che *.cfg files or passing them via cli and check that they're ignored and a default value is used
Diff Detail
Diff Detail
- Lint
Lint Skipped - Unit
Unit Tests Skipped
Event Timeline
Comment Actions
We should have a better system for config values in the first place.
We have some sanitization and type checks in options.js / `options.json', but that doesn't help with people editing config files.
Ideally we had some manifest that states which config values are allowed to be read, which defaults they have, which value types can bet set and which value range is allowed.
We could easily name a bunch of more checks that could be added, but adding these few checks wouldn't hurt I guess.
Comment Actions
There is still a problem:
let scale = +Engine.ConfigDB_GetValue("user", "gui.scale");
You get the config value, but in C++ you've changed only global variables and this line still will return an invalid value, so config data should be updated somehow too.
Comment Actions
I had to add another level to CConfigDB to overcome what @vladislavbelov pointed out. Now the sanitized values should be propagated to JavaScript.
| source/ps/ConfigDB.h | ||
|---|---|---|
| 44 | What's a reason of adding this? | |
| 111 | As you can see above, people prefer to use "classic" names for types, SetValueBool/FromInt, I don't really mind, if leave as is, but to be convinced, I want to someone take a look at it too. | |
| source/ps/GameSetup/Config.cpp | ||
| 193 | ps/VideoMode.cpp has defined default values and methods for getting default desktop resolution, couldn't we use it instead of this hard-coded values? | |
| source/ps/ConfigDB.h | ||
|---|---|---|
| 44 | Before adding this the most important CFG was CFG_COMMAND, but the config variables checking is more important than CLI arguments (and also setting them as CFG_COMMAND wouldn't make sense), so I added CFG_SANITIZE | |
Comment Actions
Nobody should need to edit a config file by hand (that they still have to do so in a few cases just shows that the options screen is incomplete). If they still want to do that they will have to take care, no hand holding.
Ideally we had some manifest that states which config values are allowed to be read, which defaults they have, which value types can bet set and which value range is allowed.
That'd be nice, but we do have that in the options screen which should be good enough. (And I'd prefer not having config files in XML.)
For the overall patch: No to the ConfigDB changes (apart from missing locking and possibly namespace checks). You should sanitize values before they are stored there, not figure out what is valid afterwards.
Comment Actions
Then I think it's better to abandon this patch since this is only meant for people who manually edit che config file.