[libxml++] Validators: Improve the error handling.

[Kjell Ahlstedt <kjellahl src gnome org>]

commit 09d2a9048191455dbd1849359949f212b906f0a3
Author: Kjell Ahlstedt <kjell ahlstedt bredband net>
Date:   Thu Aug 9 10:41:12 2012 +0200

    Validators: Improve the error handling.
    
    * libxml++/validators/validator.[h|cc]:
    * libxml++/validators/dtdvalidator.[h|cc]:
    * libxml++/validators/schemavalidator.[h|cc]: Check more return codes from
    libxml2 functions. Improve the description of member functions in the
    reference documentation. Bug #635846.

 ChangeLog                              |   10 ++++
 libxml++/validators/dtdvalidator.cc    |   44 +++++++++--------
 libxml++/validators/dtdvalidator.h     |   68 ++++++++++++++++++++++----
 libxml++/validators/schemavalidator.cc |   34 ++++++++++---
 libxml++/validators/schemavalidator.h  |   83 +++++++++++++++++++++++++++++---
 libxml++/validators/validator.cc       |   40 ++++++++++-----
 libxml++/validators/validator.h        |    3 +-
 7 files changed, 222 insertions(+), 60 deletions(-)
---
diff --git a/ChangeLog b/ChangeLog
index 6d1c1a5..ed58e7c 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,13 @@
+2012-08-09  Kjell Ahlstedt  <kjell ahlstedt bredband net>
+
+	Validators: Improve the error handling.
+
+	* libxml++/validators/validator.[h|cc]:
+	* libxml++/validators/dtdvalidator.[h|cc]:
+	* libxml++/validators/schemavalidator.[h|cc]: Check more return codes from
+	libxml2 functions. Improve the description of member functions in the
+	reference documentation. Bug #635846.
+
 2012-08-07  Kjell Ahlstedt  <kjell ahlstedt bredband net>
 
 	Add incremental parsing to the SaxParser example program.
diff --git a/libxml++/validators/dtdvalidator.cc b/libxml++/validators/dtdvalidator.cc
index f000214..133c75e 100644
--- a/libxml++/validators/dtdvalidator.cc
+++ b/libxml++/validators/dtdvalidator.cc
@@ -7,15 +7,13 @@
 
 #include "libxml++/validators/dtdvalidator.h"
 #include "libxml++/dtd.h"
-#include "libxml++/nodes/element.h"
-#include "libxml++/nodes/textnode.h"
-#include "libxml++/nodes/commentnode.h"
-#include "libxml++/keepblanks.h"
+#include "libxml++/nodes/node.h"
 #include "libxml++/exceptions/internal_error.h"
+#include "libxml++/exceptions/validity_error.h"
 #include "libxml++/io/istreamparserinputbuffer.h"
 #include "libxml++/document.h"
 
-#include <libxml/parserInternals.h>//For xmlCreateFileParserCtxt().
+#include <libxml/parser.h>
 
 #include <sstream>
 #include <iostream>
@@ -54,6 +52,7 @@ void DtdValidator::parse_file(const Glib::ustring& filename)
 void DtdValidator::parse_subset(const Glib::ustring& external,const Glib::ustring& system)
 {
   release_underlying(); // Free any existing dtd.
+  xmlResetLastError();
 
   xmlDtd* dtd = xmlParseDTD(
     external.empty() ? 0 : (const xmlChar *)external.c_str(),
@@ -61,7 +60,7 @@ void DtdValidator::parse_subset(const Glib::ustring& external,const Glib::ustrin
 
   if (!dtd)
   {
-    throw parse_error("Dtd could not be parsed");
+    throw parse_error("Dtd could not be parsed.\n" + format_xml_error());
   }
 
   Node::create_wrapper(reinterpret_cast<xmlNode*>(dtd));
@@ -78,7 +77,8 @@ void DtdValidator::parse_memory(const Glib::ustring& contents)
 
 void DtdValidator::parse_stream(std::istream& in)
 {
-  release_underlying(); //Free any existing document.
+  release_underlying(); // Free any existing dtd.
+  xmlResetLastError();
 
   IStreamParserInputBuffer ibuff( in );
 
@@ -86,7 +86,7 @@ void DtdValidator::parse_stream(std::istream& in)
 
   if (!dtd)
   {
-    throw parse_error("Dtd could not be parsed");
+    throw parse_error("Dtd could not be parsed.\n" + format_xml_error());
   }
 
   Node::create_wrapper(reinterpret_cast<xmlNode*>(dtd));
@@ -97,9 +97,9 @@ void DtdValidator::release_underlying()
 {
   if(dtd_)
   {
-    //Make a local copy as the wrapper is destroyed first
+    //Make a local pointer to the underlying xmlDtd object as the wrapper is destroyed first.
     //After free_wrappers is called dtd_ will be invalid (e.g. delete dtd_)
-    xmlDtd* dtd=dtd_->cobj();
+    xmlDtd* dtd = dtd_->cobj();
     Node::free_wrappers(reinterpret_cast<xmlNode*>(dtd));
     xmlFreeDtd(dtd);
     dtd_ = 0;
@@ -123,33 +123,37 @@ const Dtd* DtdValidator::get_dtd() const
 
 bool DtdValidator::validate(const Document* doc)
 {
+  if (!doc)
+  {
+    throw internal_error("Document pointer cannot be 0.");
+  }
+
+  if (!dtd_)
+  {
+    throw internal_error("No DTD to use for validation.");
+  }
+
   // A context is required at this stage only
   if (!valid_)
     valid_ = xmlNewValidCtxt();
 
   if(!valid_)
   {
-    throw internal_error("Couldn't create parsing context");
-  }
-
-  if (!doc)
-  {
-    throw internal_error("Document pointer cannot be 0");
+    throw internal_error("Couldn't create validation context");
   }
 
+  xmlResetLastError();
   initialize_valid();
 
   const bool res = (bool)xmlValidateDtd( valid_, (xmlDoc*)doc->cobj(), dtd_->cobj() );
 
-  if(res == 0)
+  if (!res)
   {
     check_for_exception();
-
-    throw validity_error("Document failed Dtd validation");
+    throw validity_error("Document failed DTD validation\n" + format_xml_error());
   }
 
   return res;
 }
 
 } // namespace xmlpp
-
diff --git a/libxml++/validators/dtdvalidator.h b/libxml++/validators/dtdvalidator.h
index 674e7af..863faa0 100644
--- a/libxml++/validators/dtdvalidator.h
+++ b/libxml++/validators/dtdvalidator.h
@@ -5,8 +5,8 @@
  * included with libxml++ as the file COPYING.
  */
 
-#ifndef __LIBXMLPP_PARSERS_DTDVALIDATOR_H
-#define __LIBXMLPP_PARSERS_DTDVALIDATOR_H
+#ifndef __LIBXMLPP_VALIDATORS_DTDVALIDATOR_H
+#define __LIBXMLPP_VALIDATORS_DTDVALIDATOR_H
 
 #include <libxml++/validators/validator.h>
 #include <libxml++/dtd.h>
@@ -14,28 +14,79 @@
 
 namespace xmlpp {
 
-/** XML DOM parser.
- *
+/** XML DTD validator.
  */
 class DtdValidator : public Validator
 {
 public:
   DtdValidator();
+
+  /** Create a validator and parse an external subset (DTD file) immediately.
+   * @param file The URL of the DTD.
+   * @throws xmlpp::parse_error
+   */
   explicit DtdValidator(const Glib::ustring& file);
+
+  /** Create a validator and parse an external subset (DTD file) immediately.
+   * @param external The external ID of the DTD.
+   * @param system The URL of the DTD.
+   * @throws xmlpp::parse_error
+   */
   explicit DtdValidator(const Glib::ustring& external,const Glib::ustring& system);
+
   virtual ~DtdValidator();
 
+  /** Parse an external subset (DTD file).
+   * If the validator already contains a DTD, that DTD is deleted.
+   * @param external The external ID of the DTD.
+   * @param system The URL of the DTD.
+   * @throws xmlpp::parse_error
+   */
   virtual void parse_subset(const Glib::ustring& external,const Glib::ustring& system);
+
+  /** Parse an external subset (DTD file).
+   * If the validator already contains a DTD, that DTD is deleted.
+   * @param filename The URL of the DTD.
+   * @throws xmlpp::parse_error
+   */
   virtual void parse_file(const Glib::ustring& filename);
+
+  /** Parse a DTD from a string.
+   * If the validator already contains a DTD, that DTD is deleted.
+   * @param contents The DTD as a string.
+   * @throws xmlpp::parse_error
+   */
   virtual void parse_memory(const Glib::ustring& contents);
+
+  /** Parse a DTD from a stream.
+   * If the validator already contains a DTD, that DTD is deleted.
+   * @param in The stream.
+   * @throws xmlpp::parse_error
+   */
   virtual void parse_stream(std::istream& in);
 
-  /** Test whether a document has been parsed.
-  */
+  /** Test whether a DTD has been parsed.
+   */
   operator bool() const;
+
+  /** Get the parsed DTD.
+   * @returns A pointer to the parsed DTD, or <tt>0</tt>.
+   */
   Dtd* get_dtd();
+
+  /** Get the parsed DTD.
+   * @returns A pointer to the parsed DTD, or <tt>0</tt>.
+   */
   const Dtd* get_dtd() const;
 
+  /** Validate a document, using a previously parsed DTD.
+   * The internal subset (if present) is de-coupled (i.e. not used),
+   * which could give problems if ID or IDREF is present.
+   * @param doc Pointer to the document.
+   * @returns Whether the document is valid.
+   * @throws xmlpp::internal_error
+   * @throws xmlpp::validity_error
+   */
   bool validate(const Document* doc);
 
 protected:
@@ -44,10 +95,7 @@ protected:
   Dtd* dtd_;
 };
 
-
-
-
 } // namespace xmlpp
 
-#endif //__LIBXMLPP_PARSERS_DTDVALIDATOR_H
+#endif //__LIBXMLPP_VALIDATORS_DTDVALIDATOR_H
 
diff --git a/libxml++/validators/schemavalidator.cc b/libxml++/validators/schemavalidator.cc
index 2a73640..c51b62c 100644
--- a/libxml++/validators/schemavalidator.cc
+++ b/libxml++/validators/schemavalidator.cc
@@ -71,11 +71,14 @@ SchemaValidator::~SchemaValidator()
 
 void SchemaValidator::parse_context(_xmlSchemaParserCtxt* context)
 {
-  release_underlying(); // Free any existing dtd.
+  if (!context)
+    throw parse_error("Could not create schema parser context\n" + format_xml_error());
+
+  release_underlying(); // Free any existing schema.
 
   xmlSchema* schema = xmlSchemaParse( context );
   if ( ! schema )
-   throw parse_error("Schema could not be parsed");
+    throw parse_error("Schema could not be parsed\n" + format_xml_error());
 
   schema->_private = new Schema(schema);
 
@@ -85,6 +88,7 @@ void SchemaValidator::parse_context(_xmlSchemaParserCtxt* context)
 
 void SchemaValidator::parse_file(const Glib::ustring& filename)
 {
+  xmlResetLastError();
   xmlSchemaParserCtxtPtr ctx = xmlSchemaNewParserCtxt( filename.c_str() );
   XmlSchemaParserContextHolder holder(ctx);
   parse_context( ctx );
@@ -92,6 +96,7 @@ void SchemaValidator::parse_file(const Glib::ustring& filename)
 
 void SchemaValidator::parse_memory(const Glib::ustring& contents)
 {
+  xmlResetLastError();
   xmlSchemaParserCtxtPtr ctx = xmlSchemaNewMemParserCtxt( contents.c_str(), contents.bytes() );
   XmlSchemaParserContextHolder holder(ctx);
   parse_context( ctx );
@@ -99,6 +104,7 @@ void SchemaValidator::parse_memory(const Glib::ustring& contents)
 
 void SchemaValidator::parse_document(Document& document)
 {
+  xmlResetLastError();
   xmlSchemaParserCtxtPtr ctx = xmlSchemaNewDocParserCtxt( document.cobj() );
   XmlSchemaParserContextHolder holder(ctx);
   parse_context( ctx );
@@ -168,17 +174,22 @@ bool SchemaValidator::validate(const Document* doc)
     throw internal_error("Couldn't create validating context");
   }
 
+  xmlResetLastError();
   initialize_valid();
 
-  int res = xmlSchemaValidateDoc( ctxt_, (xmlDoc*)doc->cobj() );
+  const int res = xmlSchemaValidateDoc( ctxt_, (xmlDoc*)doc->cobj() );
 
   if(res != 0)
   {
     check_for_exception();
-    throw validity_error("Document failed schema validation");
+
+    Glib::ustring error_str = format_xml_error();
+    if (error_str.empty())
+      error_str = "Error code from xmlSchemaValidateDoc(): " + Glib::ustring::format(res);
+    throw validity_error("Document failed schema validation\n" + error_str);
   }
 
-  return res;
+  return res == 0;
 }
 
 bool SchemaValidator::validate(const Glib::ustring& file)
@@ -188,6 +199,7 @@ bool SchemaValidator::validate(const Glib::ustring& file)
 
   if (!schema_)
     throw internal_error("Must have a schema to validate document");
+
   // A context is required at this stage only
   if (!ctxt_)
     ctxt_ = xmlSchemaNewValidCtxt( schema_->cobj() );
@@ -196,17 +208,23 @@ bool SchemaValidator::validate(const Glib::ustring& file)
   {
     throw internal_error("Couldn't create validating context");
   }
+
+  xmlResetLastError();
   initialize_valid();
 
-  int res = xmlSchemaValidateFile( ctxt_, file.c_str(), 0 );
+  const int res = xmlSchemaValidateFile( ctxt_, file.c_str(), 0 );
 
   if(res != 0)
   {
     check_for_exception();
-    throw validity_error("Document failed schema validation");
+
+    Glib::ustring error_str = format_xml_error();
+    if (error_str.empty())
+      error_str = "Error code from xmlSchemaValidateFile(): " + Glib::ustring::format(res);
+    throw validity_error("Document failed schema validation\n" + error_str);
   }
 
-  return res;
+  return res == 0;
 }
 
 } // namespace xmlpp
diff --git a/libxml++/validators/schemavalidator.h b/libxml++/validators/schemavalidator.h
index c3aa7c9..f1ee910 100644
--- a/libxml++/validators/schemavalidator.h
+++ b/libxml++/validators/schemavalidator.h
@@ -21,7 +21,7 @@ extern "C" {
 
 namespace xmlpp {
 
-/** Schema Validator
+/** XML Schema Validator.
  *
  * @newin{2,24}
  */
@@ -29,23 +29,95 @@ class SchemaValidator : public Validator
 {
 public:
   SchemaValidator();
+
+  /** Create a validator and parse a schema definition file immediately.
+   * @param file The URL of the schema.
+   * @throws xmlpp::parse_error
+   */
   explicit SchemaValidator(const Glib::ustring& file);
+
+  /** Create a validator and parse a schema definition document immediately.
+   * @param document A preparsed document tree, containing the schema definition.
+   * @note The document may be modified during the parsing process.
+   * @throws xmlpp::parse_error
+   */
   explicit SchemaValidator(Document& document);
+
+  /** Create a schema validator.
+   * @param schema A pointer to the XML schema to use when validating XML documents.
+   *        The validator does not take ownership of the schema. The caller must
+   *        guarantee that the schema exists as long as the validator keeps a
+   *        pointer to it. The caller is responsible for deleting the schema
+   *        when it's no longer needed.
+   */
   explicit SchemaValidator(Schema* schema);
+
   virtual ~SchemaValidator();
 
+  /** Parse a schema definition file.
+   * If the validator already contains a schema, that schema is released
+   * (deleted if the validator owns the schema).
+   * @param file The URL of the schema.
+   * @throws xmlpp::parse_error
+   */
   virtual void parse_file(const Glib::ustring& filename);
+
+  /** Parse a schema definition from a string.
+   * If the validator already contains a schema, that schema is released
+   * (deleted if the validator owns the schema).
+   * @param contents The schema definition as a string.
+   * @throws xmlpp::parse_error
+   */
   virtual void parse_memory(const Glib::ustring& contents);
+
+  /** Parse a schema definition from a document.
+   * If the validator already contains a schema, that schema is released
+   * (deleted if the validator owns the schema).
+   * @param document A preparsed document tree, containing the schema definition.
+   * @note The document may be modified during the parsing process.
+   * @throws xmlpp::parse_error
+   */
   virtual void parse_document(Document& document);
+
+  /** Set a schema.
+   * If the validator already contains a schema, that schema is released
+   * (deleted if the validator owns the schema).
+   * @param schema A pointer to the XML schema to use when validating XML documents.
+   *        The validator does not take ownership of the schema. The caller must
+   *        guarantee that the schema exists as long as the validator keeps a
+   *        pointer to it. The caller is responsible for deleting the schema
+   *        when it's no longer needed.
+   */
   virtual void set_schema(Schema* schema);
 
-  /** Test whether a document has been parsed.
-  */
+  /** Test whether a schema has been parsed.
+   */
   operator bool() const;
+
+  /** Get the parsed schema.
+   * @returns A pointer to the parsed schema, or <tt>0</tt>.
+   */
   Schema* get_schema();
+
+  /** Get the parsed schema.
+   * @returns A pointer to the parsed schema, or <tt>0</tt>.
+   */
   const Schema* get_schema() const;
 
+  /** Validate a document, using a previously parsed schema.
+   * @param doc Pointer to the document.
+   * @returns Whether the document is valid.
+   * @throws xmlpp::internal_error
+   * @throws xmlpp::validity_error
+   */
   bool validate(const Document* doc);
+
+  /** Validate an XML file, using a previously parsed schema.
+   * @param file The URI of the XML file.
+   * @returns Whether the document is valid.
+   * @throws xmlpp::internal_error
+   * @throws xmlpp::validity_error
+   */
   bool validate(const Glib::ustring& file);
 
 protected:
@@ -54,13 +126,10 @@ protected:
   virtual void release_underlying();
 
   Schema* schema_;
-  bool embbeded_shema_;
+  bool embbeded_shema_; //TODO Correct mis-spelling at the next API/ABI break.
   _xmlSchemaValidCtxt* ctxt_;
 };
 
-
-
-
 } // namespace xmlpp
 
 #endif //__LIBXMLPP_VALIDATOR_SCHEMAVALIDATOR_H
diff --git a/libxml++/validators/validator.cc b/libxml++/validators/validator.cc
index 119ba61..d073e16 100644
--- a/libxml++/validators/validator.cc
+++ b/libxml++/validators/validator.cc
@@ -26,8 +26,7 @@ Validator::~Validator()
 
 void Validator::initialize_valid()
 {
-  //Tell the validity valid about the callbacks:
-  //(These are only called if validation is on - see above)
+  //Tell the validation context about the callbacks:
   valid_->error = &callback_validity_error;
   valid_->warning = &callback_validity_warning;
 
@@ -64,21 +63,28 @@ void Validator::on_validity_warning(const Glib::ustring& message)
 
 void Validator::check_for_validity_messages()
 {
-  if(!validate_error_.empty())
-  {
-    if(!exception_)
-      exception_ = new validity_error("Validity error:\n" + validate_error_);
+  Glib::ustring msg(exception_ ? exception_->what() : "");
+  bool validity_msg = false;
 
+  if (!validate_error_.empty())
+  {
+    validity_msg = true;
+    msg += "\nValidity error:\n" + validate_error_;
     validate_error_.erase();
   }
 
-  if(!validate_warning_.empty())
+  if (!validate_warning_.empty())
   {
-    if(!exception_)
-      exception_ = new validity_error("Validity warning:\n" + validate_warning_);
-
+    validity_msg = true;
+    msg += "\nValidity warning:\n" + validate_warning_;
     validate_warning_.erase();
   }
+
+  if (validity_msg)
+  {
+    delete exception_;
+    exception_ = new validity_error(msg);
+  }
 }
 
 void Validator::callback_validity_error(void* valid_, const char* msg, ...)
@@ -133,9 +139,19 @@ void Validator::callback_validity_warning(void* valid_, const char* msg, ...)
 
 void Validator::handleException(const exception& e)
 {
+  delete exception_;
   exception_ = e.Clone();
 
-  release_underlying();
+  // Don't delete the DTD validation context or schema validation context
+  // while validating. It would cause accesses to deallocated memory in libxml2
+  // functions after the return from Validator::callback_validity_...().
+  // Parser::handleException() calls xmlStopParser(), but there is no
+  // xmlStopValidator() or similar function to call here.
+  // We don't throw the exception here, since it would have to pass through
+  // C functions. That's not guaranteed to work. It might work, but it depends
+  // on the C compiler and the options used when building libxml2.
+
+  //release_underlying();
 }
 
 void Validator::check_for_exception()
@@ -151,5 +167,3 @@ void Validator::check_for_exception()
 }
 
 } // namespace xmlpp
-
-
diff --git a/libxml++/validators/validator.h b/libxml++/validators/validator.h
index a7ca2b3..7dc1b90 100644
--- a/libxml++/validators/validator.h
+++ b/libxml++/validators/validator.h
@@ -22,8 +22,7 @@ extern "C" {
 
 namespace xmlpp {
 
-/** XML parser.
- *
+/** Base class for XML validators.
  */
 class Validator : NonCopyable
 {