comps: Handle errors while serializing groups and environments#2670
comps: Handle errors while serializing groups and environments#2670pkratoch wants to merge 2 commits intorpm-software-management:mainfrom
Conversation
Both `xmlNewNode` and `xmlNewProp` can return NULL. In case of adding elements with translations, we can handle this by not adding them.
Based on commits 74706c1 and d1e1e76 which dealt with the groups serialization. By default libxml2 prints errors (e.g. "permission denied") directly to stderr which interferes with dnf5 output. This patch collects all errors to make them part of the exception message later. This also fixes the memory leak that occurs because the `doc` variable is not properly freed before an error is thrown.
evan-goode
left a comment
evan-goode
left a comment
There was a problem hiding this comment.
I think Petr had in mind a bigger patch that would address more of the ignored allocation failures.
| @@ -311,7 +311,13 @@ | |||
| // If it's successful (wasn't already present), create an XML node for this translation | |||
| if (name_langs.insert(lang).second) { | |||
| node = utils::xml::add_subnode_with_text(node_environment, "name", std::string(di.kv.str)); | |||
There was a problem hiding this comment.
add_subnode_with_text should only return NULL here if allocation fails. Wouldn't we always want to throw a std::bad_alloc if allocation fails, rather than skipping the node?
There was a problem hiding this comment.
Hm, I guess I thought it would be better to store at least something if the serialization fails for the translations, which are not as critical. And I wasn't sure yet how to handle the others, but I can change it so that we throw a std::bad_alloc in every case it fails.
There was a problem hiding this comment.
yeah, that would make sense if the serialization could fail for some benign reason. But I think if any of the libxml2 functions return NULL here it will always be because of OOM, which we should panic at the first sign of. The docs mention NULL can also be returned "if arguments are invalid" but I don't know how that would be triggered since we always pass in some std::string::c_str().
Also, I missed that add_subnode_with_text is our own wrapper for libxml2 functions---instead of guarding against it returning NULL, it should be adjusted to throw std::bad_alloc itself when any of the libxml2 functions it calls returns NULL.
| xmlAddChild(node_comps, node_environment); | ||
|
|
||
| // Add id, name, description, display_order | ||
| utils::xml::add_subnode_with_text(node_environment, "id", get_environmentid()); |
There was a problem hiding this comment.
Ideally we would guard against any of the libxml2 functions returning NULL: xmlNewNode, xmlNewProp, other occurrences of add_subnode_with_text, xmlNewDoc, xmlNewText, xmlAddChild, possibly others.
| // libxml2 error handler. By default libxml2 prints errors directly to stderr which | ||
| // makes a mess of the outputs. | ||
| // This stores the errors in a vector of strings; | ||
| __attribute__((__format__(printf, 2, 0))) static void error_to_strings(void * ctx, const char * fmt, ...) { |
There was a problem hiding this comment.
We could move this to xml.hpp and share it between groups and environment.
| // This stores the errors in a vector of strings; | ||
| __attribute__((__format__(printf, 2, 0))) static void error_to_strings(void * ctx, const char * fmt, ...) { | ||
| auto xml_errors = static_cast<std::vector<std::string> *>(ctx); | ||
| char buffer[256]; |
There was a problem hiding this comment.
No big deal, this is copied from the existing implementation, but it would be better to remove the fixed-length buffer to not truncate any long messages.
|
|
||
| if (save_result == -1) { | ||
| // There can be duplicit messages in the libxml2 errors so make them unique | ||
| auto it = unique(xml_errors.begin(), xml_errors.end()); |
There was a problem hiding this comment.
std::unique only removes consecutive duplicates, you will need to sort the vector first.
| xmlSetGenericErrorFunc(NULL, NULL); | ||
|
|
||
| if (save_result == -1) { | ||
| // There can be duplicit messages in the libxml2 errors so make them unique |
2 participants
Handle errors while serializing environments:
Based on commits 74706c1 and d1e1e76 which dealt with the groups
serialization.
By default libxml2 prints errors (e.g. "permission denied") directly to
stderr which interferes with dnf5 output.
This patch collects all errors to make them part of the exception
message later.
This also fixes the memory leak that occurs because the
docvariableis not properly freed before an error is thrown.
Handle serialize errors when xml node or attribute creation fails:
Both
xmlNewNodeandxmlNewPropcan return NULL. In case of adding elementswith translations, we can handle this by not adding them.
(Discovered in #2633 )