Improve Error Reporting for Schema Validation

[JDSeiler]

I have some prior Rust experience, but I'd consider myself an intermediate Rust programmer at best, and I have very little experience with C and FFI. Point being, I'm very open to feedback on these changes.

Motivation

Closes #115

Previously, StructuredError was a thin wrapper around a raw pointer to libxml2's xmlError struct. This was problematic because libxml2 does not allocate separate xmlError structs. Instead, it uses an effectively global (or thread-global) xmlError that is rewritten (but does not move) every time a new error is generated. As a result, all of the errors produced by this library were all pointing to the same memory and all had the same contents: the last error libxml2 produced.

The following code from libxml2 was consulted to confirm its behavior:

• https://github.com/GNOME/libxml2/blob/884474477284474e0151280aaa275a18e3d7a036/error.c#L470 is the function ultimately responsible for calling the structured error callback (called schannel in libxml)
  • line 632 is where the data is actually provided to the callback. The error object is called to.
  • Walking backwards, we see the error object being cleared and initialized starting on line 573
  • Walking back even farther, we see that to is not a fresh xmlError, but rather a reference to the xmlLastError global. This either points to a file-level xmlError defined in globals.c, or a struct-member that's part of a thread-specific global context. In either case, it does not appear that xmlLastError is ever allocated more than once.

Description of Changes

I replaced the wrapper around the raw pointer with a more traditional Rust struct so that each individual error could be preserved. Because the struct no longer has any ties to the underlying C-managed data once constructed, the Drop implementation was removed.

Not all of the fields in the xmlError struct had obvious utility, or even safe ways of managing them. The fields ommitted are:

• str1, str2, str3 :: I couldn't find any indication as to what these would be used for. Perhaps they are "empty buckets" for users to put their own custom error messages into? It didn't seem worth spending the CPU cycles on looking at them.
• int1 :: Similarly, I couldn't find any information on this aside from the cryptic "extra number information".
• ctxt, node :: Both of these are void * and I have 0 clue how to safely include them without massively complicating things.

Regarding enums, the xmlErrorLevel field was converted to a normal Rust enum because it's small. The code and domain fields were included in their raw forms because they might be useful to someone. However, the xmlErrorDomain and xmlParserError C enums are 30 and 700+ (!!) members respectively, so it didn't seem wise to write out Rust enum versions of them.

Draft -> Ready for Review Checklist

• [x] :: What should the fallback value for the XmlErrorLevel::from_raw function be? Do we trust that libxml2 is always going to provide something in the range [0, 3], and the catch-all branch can be something like unreachable!()?
• [x] :: Is the type Option<String> acceptable for the message and filename fields? What about the lossy conversion to utf-8? I went through a few different options here. The underlying C string could be a null pointer (at least for filename) and of course there's no guarantee that the underlying C string is valid utf-8.
• [x] :: Any other finishing tasks I need to do? Run Valgrind? Improve documentation? I'm all ears.

[dginev]

@JDSeiler thank you for the thorough work in this PR, and thanks to @imcsk8 for the unexpected review. This is already more than the usual PR burden on this wrapper repo, I appreciate the contributions. Maybe it is a sign of the crate starting to mature.

As to the questions by @JDSeiler :

• I think 0..=3 is indeed a reliably documented range for xmlErrorLevel, so your suggestion to make other cases unreachable!() is the correct design to signal that expectation.

• Running valgrind on the updated test is a good idea, though we haven't done it consistently in the past. The usual run of

  valgrind --leak-check=full ./target/debug/deps/schema_tests-1dd74906a490bf98

  seems to indicate that there are no issues with the new test on a local build here.

• The Option<String> is fine in general, the crate uses that return type on occasion. But I would also honor the request by @imcsk8 and keep the original message around as #[deprecated] from version 0.3.3. We can keep it around until 0.4.0 - although if someone wants to sink in some time to improve the crate, I am more than happy to recruit co-maintainers, and have others speak into the versioning roadmap.

• clippy also caught that StructuredError::from_raw should be marked as an unsafe function, see not_unsafe_ptr_arg_deref

• I usually also update CHANGELOG.md in the crate root when a new PR gets merged - I can do that after merging here, or you're welcome to update it yourself if you want to choose the exact wording.

---

Aside: What actually stuck out to me is libxml's decision to add trailing \n newline chars to the error values. I am a bit uneasy about exposing those from the Rust struct, without first running a .trim() on them, especially if we are allocating a String anyway. But I could see this going either way.

[JDSeiler]

I left a small type suggestion, and will wait for a final nod from @imcsk8 , but we're pretty much good to merge and ship a v0.3.3 here.

Thanks again @JDSeiler

Awesome, thank you (and @imcsk8 !) for feedback on the changes. This was a great learning experience!

[imcsk8]

... I am more than happy to recruit co-maintainers, and have others speak into the versioning roadmap.

I'll be happy to help maintaining. What do you need me to do?

[dginev]

@imcsk8 basically what you just did in reviewing here, whenever you have time/interest.

I am currently available enough to ship minor releases, but I have been a little easy-going with spending time in developing the crate. Btw this is also how @triptec got added in here some time back, when he had a season of libxml work.

And since this is currently a care-free open source crate you can also just sit on the rights without doing anything, it keeps the crate safer in case I'm missing for whatever reasons.

[dginev]

@imcsk8 and in the interest of speed, I just added you as an admin to the repo (you should get an invite).

Feel free to merge this PR in as a warm-up, if the general setup sounds reasonable enough.