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.

Sign up for GitHub

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

Breakup load into separate functions #360

Closed
wants to merge 1 commit into from
+17 −7
Closed

Breakup load into separate functions #360

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 16 additions & 7 deletions src/Homie/Config.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,27 +17,25 @@ bool Config::_spiffsBegin() {
return _spiffsBegan;
}

bool Config::load() {
if (!_spiffsBegin()) { return false; }

_valid = false;
JsonObject& Config::loadJSONObject() {
if (!_spiffsBegin()) { return JsonObject::invalid(); }

if (!SPIFFS.exists(CONFIG_FILE_PATH)) {
Interface::get().getLogger() << F("✖ ") << CONFIG_FILE_PATH << F(" doesn't exist") << endl;
return false;
return JsonObject::invalid();
}

File configFile = SPIFFS.open(CONFIG_FILE_PATH, "r");
if (!configFile) {
Interface::get().getLogger() << F("✖ Cannot open config file") << endl;
return false;
return JsonObject::invalid();
}

size_t configSize = configFile.size();

if (configSize > MAX_JSON_CONFIG_FILE_SIZE) {
Interface::get().getLogger() << F("✖ Config file too big") << endl;
return false;
return JsonObject::invalid();
}

char buf[MAX_JSON_CONFIG_FILE_SIZE];
Expand All @@ -49,6 +47,17 @@ bool Config::load() {
JsonObject& parsedJson = jsonBuffer.parseObject(buf);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

jsonBuffer is a local variable, so you create a reference to something internal to this local variable.
The local variable gets destroyed when leaving Config::load(), this (hopefully ;-) frees the buffer of parsedJson.

if (!parsedJson.success()) {
Interface::get().getLogger() << F("✖ Invalid JSON in the config file") << endl;
return JsonObject::invalid();
}
return parsedJson;
}


bool Config::load() {
_valid = false;
JsonObject& parsedJson = Config::loadJSONObject();
if (!parsedJson.success()) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

parsedJson is freed memory, so behaviour is undefined. (It will work in most cases anyhow, because no one else has overwritten the memory region yet... but technically this is just good luck....)

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's been a decade or two since I've done any real C++ coding. Thanks for the sanity check.

// Config::loadJSONObject would already log a specific error so we don't need to here
return false;
}

Expand Down
1 change: 1 addition & 0 deletions src/Homie/Config.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ namespace HomieInternals {
class Config {
public:
Config();
JsonObject& loadJSONObject();
bool load();
inline const ConfigStruct& get() const;
char* getSafeConfigFile() const;
Expand Down