comps: Handle errors while serializing groups and environments

libdnf5/comps/environment/environment.cpp

@@ -20,6 +20,7 @@
 #include "libdnf5/comps/environment/environment.hpp"
 
 #include "solv/pool.hpp"
+#include "utils/string.hpp"
 #include "utils/xml.hpp"
 
 #include "libdnf5/base/base.hpp"
@@ -38,6 +39,7 @@ extern "C" {
 #include <libxml/tree.h>
 #include <limits.h>
 
+#include <memory>
 #include <set>
 #include <string>
 #include <string_view>
@@ -268,14 +270,20 @@ bool Environment::get_installed() const {
 
 
 void Environment::serialize(const std::string & path) {
+    std::vector<std::string> xml_errors;
+    utils::xml::GenericErrorFuncGuard error_guard(&xml_errors, &utils::xml::error_to_strings);
+
     // Create doc with root node "comps"
-    xmlDocPtr doc = xmlNewDoc(BAD_CAST "1.0");
-    xmlNodePtr node_comps = xmlNewNode(NULL, BAD_CAST "comps");
-    xmlDocSetRootElement(doc, node_comps);
+    std::unique_ptr<xmlDoc, utils::xml::XmlDocDeleter> doc(xmlNewDoc(BAD_CAST "1.0"));
+    if (!doc) {
+        throw std::bad_alloc();
+    }
+    xmlNodePtr node_comps = utils::xml::new_node("comps");
+    xmlDocSetRootElement(doc.get(), node_comps);
 
     // Create "environment" node
-    xmlNodePtr node_environment = xmlNewNode(NULL, BAD_CAST "environment");
-    xmlAddChild(node_comps, node_environment);
+    xmlNodePtr node_environment = utils::xml::new_node("environment");
+    utils::xml::add_child(node_comps, node_environment);
 
     // Add id, name, description, display_order
     utils::xml::add_subnode_with_text(node_environment, "id", get_environmentid());
@@ -311,7 +319,7 @@ void Environment::serialize(const std::string & path) {
                 // 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));
-                    xmlNewProp(node, BAD_CAST "xml:lang", BAD_CAST lang.c_str());
+                    utils::xml::new_prop(node, "xml:lang", lang);
                 }
             }
             // If keyname starts with "solvable:description:", it's a description translation
@@ -321,39 +329,40 @@ void Environment::serialize(const std::string & path) {
                 // If it's successful (wasn't already present), create an XML node for this translation
                 if (description_langs.insert(lang).second) {
                     node = utils::xml::add_subnode_with_text(node_environment, "description", std::string(di.kv.str));
-                    xmlNewProp(node, BAD_CAST "xml:lang", BAD_CAST lang.c_str());
+                    utils::xml::new_prop(node, "xml:lang", lang);
                 }
             }
         }
         dataiterator_free(&di);
     }
 
     // Add grouplist
-    xmlNodePtr node_grouplist = xmlNewNode(NULL, BAD_CAST "grouplist");
-    xmlAddChild(node_environment, node_grouplist);
+    xmlNodePtr node_grouplist = utils::xml::new_node("grouplist");
+    utils::xml::add_child(node_environment, node_grouplist);
     for (const auto & group : get_groups()) {
         // Create an XML node for this group
         node = utils::xml::add_subnode_with_text(node_grouplist, "groupid", group);
     }
-    xmlNodePtr node_optionlist = xmlNewNode(NULL, BAD_CAST "optionlist");
-    xmlAddChild(node_environment, node_optionlist);
+    xmlNodePtr node_optionlist = utils::xml::new_node("optionlist");
+    utils::xml::add_child(node_environment, node_optionlist);
     for (const auto & group : get_default_groups()) {
         // Create an XML node for this group
         node = utils::xml::add_subnode_with_text(node_optionlist, "groupid", group);
-        xmlNewProp(node, BAD_CAST "default", BAD_CAST "true");
+        utils::xml::new_prop(node, "default", "true");
     }
     for (const auto & group : get_optional_groups()) {
         // Create an XML node for this group
         node = utils::xml::add_subnode_with_text(node_optionlist, "groupid", group);
     }
 
     // Save the document
-    if (xmlSaveFormatFileEnc(path.c_str(), doc, "utf-8", 1) == -1) {
-        throw utils::xml::XMLSaveError(M_("failed to save xml document for comps"));
+    if (xmlSaveFormatFileEnc(path.c_str(), doc.get(), "utf-8", 1) == -1) {
+        throw utils::xml::XMLSaveError(
+            M_("Failed to save xml document for environment \"{}\" to file \"{}\": {}"),
+            get_environmentid(),
+            path,
+            libdnf5::utils::string::join(utils::xml::make_errors_unique(std::move(xml_errors)), ", "));
     }
-
-    // Memory free
-    xmlFreeDoc(doc);
 }
 
 void Environment::add_environment_id(const EnvironmentId & environment_id) {

libdnf5/comps/group/group.cpp

@@ -37,9 +37,9 @@ extern "C" {
 }
 
 #include <libxml/tree.h>
-#include <libxml/xmlerror.h>
 #include <limits.h>
 
+#include <memory>
 #include <set>
 #include <string>
 #include <vector>
@@ -266,31 +266,21 @@ bool Group::get_installed() const {
 }
 
 
-// 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, ...) {
-    auto xml_errors = static_cast<std::vector<std::string> *>(ctx);
-    char buffer[256];
-    va_list args;
-    va_start(args, fmt);
-    vsnprintf(buffer, 256, fmt, args);
-    va_end(args);
-    xml_errors->push_back(buffer);
-}
-
 void Group::serialize(const std::string & path) {
     std::vector<std::string> xml_errors;
-    xmlSetGenericErrorFunc(&xml_errors, &error_to_strings);
+    utils::xml::GenericErrorFuncGuard error_guard(&xml_errors, &utils::xml::error_to_strings);
 
     // Create doc with root node "comps"
-    xmlDocPtr doc = xmlNewDoc(BAD_CAST "1.0");
-    xmlNodePtr node_comps = xmlNewNode(NULL, BAD_CAST "comps");
-    xmlDocSetRootElement(doc, node_comps);
+    std::unique_ptr<xmlDoc, utils::xml::XmlDocDeleter> doc(xmlNewDoc(BAD_CAST "1.0"));
+    if (!doc) {
+        throw std::bad_alloc();
+    }
+    xmlNodePtr node_comps = utils::xml::new_node("comps");
+    xmlDocSetRootElement(doc.get(), node_comps);
 
     // Create "group" node
-    xmlNodePtr node_group = xmlNewNode(NULL, BAD_CAST "group");
-    xmlAddChild(node_comps, node_group);
+    xmlNodePtr node_group = utils::xml::new_node("group");
+    utils::xml::add_child(node_comps, node_group);
 
     // Add id, name, description, default, uservisible, display_order, langonly
     utils::xml::add_subnode_with_text(node_group, "id", get_groupid());
@@ -329,7 +319,7 @@ void Group::serialize(const std::string & path) {
                 // 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_group, "name", std::string(di.kv.str));
-                    xmlNewProp(node, BAD_CAST "xml:lang", BAD_CAST lang.c_str());
+                    utils::xml::new_prop(node, "xml:lang", lang);
                 }
             }
             // If keyname starts with "solvable:description:", it's a description translation
@@ -339,42 +329,32 @@ void Group::serialize(const std::string & path) {
                 // If it's successful (wasn't already present), create an XML node for this translation
                 if (description_langs.insert(lang).second) {
                     node = utils::xml::add_subnode_with_text(node_group, "description", std::string(di.kv.str));
-                    xmlNewProp(node, BAD_CAST "xml:lang", BAD_CAST lang.c_str());
+                    utils::xml::new_prop(node, "xml:lang", lang);
                 }
             }
         }
         dataiterator_free(&di);
     }
 
     // Add packagelist
-    xmlNodePtr node_packagelist = xmlNewNode(NULL, BAD_CAST "packagelist");
-    xmlAddChild(node_group, node_packagelist);
+    xmlNodePtr node_packagelist = utils::xml::new_node("packagelist");
+    utils::xml::add_child(node_group, node_packagelist);
     for (const auto & pkg : get_packages()) {
         // Create an XML node for this package
         node = utils::xml::add_subnode_with_text(node_packagelist, "packagereq", pkg.get_name());
-        xmlNewProp(node, BAD_CAST "type", BAD_CAST pkg.get_type_string().c_str());
+        utils::xml::new_prop(node, "type", pkg.get_type_string());
         if (pkg.get_type() == PackageType::CONDITIONAL) {
-            xmlNewProp(node, BAD_CAST "requires", BAD_CAST pkg.get_condition().c_str());
+            utils::xml::new_prop(node, "requires", pkg.get_condition());
         }
     }
 
     // Save the document
-    auto save_result = xmlSaveFormatFileEnc(path.c_str(), doc, "utf-8", 1);
-
-    // Memory free
-    xmlFreeDoc(doc);
-    // reset the error handler to default
-    xmlSetGenericErrorFunc(NULL, NULL);
-
-    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());
-        xml_errors.resize(static_cast<size_t>(distance(xml_errors.begin(), it)));
+    if (xmlSaveFormatFileEnc(path.c_str(), doc.get(), "utf-8", 1) == -1) {
         throw utils::xml::XMLSaveError(
             M_("Failed to save xml document for group \"{}\" to file \"{}\": {}"),
             get_groupid(),
             path,
-            libdnf5::utils::string::join(xml_errors, ", "));
+            libdnf5::utils::string::join(utils::xml::make_errors_unique(std::move(xml_errors)), ", "));
     }
 }
 

libdnf5/utils/xml.cpp

@@ -22,16 +22,67 @@
 
 #include <libxml/tree.h>
 
+#include <algorithm>
 #include <string>
 
 
 namespace libdnf5::utils::xml {
 
 
-xmlNodePtr add_subnode_with_text(xmlNodePtr parent, std::string child_name, std::string child_text) {
-    xmlNodePtr node = xmlNewNode(NULL, BAD_CAST child_name.c_str());
-    xmlAddChild(parent, node);
-    xmlAddChild(node, xmlNewText(BAD_CAST child_text.c_str()));
+__attribute__((__format__(printf, 2, 0))) void error_to_strings(void * ctx, const char * fmt, ...) {
+    auto xml_errors = static_cast<std::vector<std::string> *>(ctx);
+    char buffer[256];
+    va_list args;
+    va_start(args, fmt);
+    vsnprintf(buffer, 256, fmt, args);
+    va_end(args);
+    xml_errors->push_back(buffer);
+}
+
+
+std::vector<std::string> make_errors_unique(std::vector<std::string> xml_errors) {
+    std::sort(xml_errors.begin(), xml_errors.end());
+    auto it = std::unique(xml_errors.begin(), xml_errors.end());
+    xml_errors.resize(static_cast<size_t>(std::distance(xml_errors.begin(), it)));
+    return xml_errors;
+}
+
+
+xmlNodePtr new_node(const std::string & node_name) {
+    xmlNodePtr node = xmlNewNode(NULL, BAD_CAST node_name.c_str());
+    if (!node) {
+        throw std::bad_alloc();
+    }
+    return node;
+}
+
+
+xmlAttrPtr new_prop(xmlNodePtr node, const std::string & name, const std::string & value) {
+    xmlAttrPtr prop = xmlNewProp(node, BAD_CAST name.c_str(), BAD_CAST value.c_str());
+    if (!prop) {
+        throw std::bad_alloc();
+    }
+    return prop;
+}
+
+
+xmlNodePtr add_child(xmlNodePtr parent, xmlNodePtr child) {
+    libdnf_assert(parent && child && parent != child, "Invalid parent or child node");
+    if (!xmlAddChild(parent, child)) {
+        throw std::bad_alloc();
+    }
+    return child;
+}
+
+
+xmlNodePtr add_subnode_with_text(xmlNodePtr parent, const std::string & child_name, const std::string & child_text) {
+    xmlNodePtr node = new_node(child_name);
+    add_child(parent, node);
+    xmlNodePtr text = xmlNewText(BAD_CAST child_text.c_str());
+    if (!text) {
+        throw std::bad_alloc();
+    }
+    add_child(node, text);
     return node;
 }
 

libdnf5/utils/xml.hpp

@@ -23,8 +23,10 @@
 #include "libdnf5/common/exception.hpp"
 
 #include <libxml/tree.h>
+#include <libxml/xmlerror.h>
 
 #include <string>
+#include <vector>
 
 
 namespace libdnf5::utils::xml {
@@ -36,7 +38,47 @@ struct XMLSaveError : public Error {
 };
 
 
-xmlNodePtr add_subnode_with_text(xmlNodePtr parent, std::string child_name, std::string child_text);
+/// Restore the default generic error handler when destroyed.
+struct GenericErrorFuncGuard {
+    GenericErrorFuncGuard(void * ctx, xmlGenericErrorFunc handler) noexcept { xmlSetGenericErrorFunc(ctx, handler); }
+    ~GenericErrorFuncGuard() { xmlSetGenericErrorFunc(NULL, NULL); }
+
+    GenericErrorFuncGuard(const GenericErrorFuncGuard &) = delete;
+    GenericErrorFuncGuard & operator=(const GenericErrorFuncGuard &) = delete;
+    GenericErrorFuncGuard(GenericErrorFuncGuard &&) = delete;
+    GenericErrorFuncGuard & operator=(GenericErrorFuncGuard &&) = delete;
+};
+
+
+// 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))) void error_to_strings(void * ctx, const char * fmt, ...);
+
+
+// There can be duplicate messages in the libxml2 errors, so make them unique.
+std::vector<std::string> make_errors_unique(std::vector<std::string> xml_errors);
+
+
+struct XmlDocDeleter {
+    void operator()(xmlDoc * doc) const noexcept {
+        if (doc != nullptr) {
+            xmlFreeDoc(doc);
+        }
+    }
+};
+
+
+xmlNodePtr new_node(const std::string & node_name);
+
+
+xmlAttrPtr new_prop(xmlNodePtr node, const std::string & name, const std::string & value);
+
+
+xmlNodePtr add_child(xmlNodePtr parent, xmlNodePtr child);
+
+
+xmlNodePtr add_subnode_with_text(xmlNodePtr parent, const std::string & child_name, const std::string & child_text);
 
 }  // namespace libdnf5::utils::xml