Re: [PATCH] Add CANONICAL option to xmlserialize

On Fri, 17 Mar 2023 at 18:01, Jim Jones <jim.jones@uni-muenster.de> wrote:
>
> After some more testing I realized that v5 was leaking the xmlDocPtr.
>
> Now fixed in v6.

Few comments:
1) Why the default option was chosen without comments shouldn't it be
the other way round?
+opt_xml_serialize_format:
+                       INDENT
                         { $$ = XMLSERIALIZE_INDENT; }
+                       | NO INDENT
                         { $$ = XMLSERIALIZE_NO_FORMAT; }
+                       | CANONICAL
                         { $$ = XMLSERIALIZE_CANONICAL; }
+                       | CANONICAL WITH NO COMMENTS
 { $$ = XMLSERIALIZE_CANONICAL; }
+                       | CANONICAL WITH COMMENTS
         { $$ = XMLSERIALIZE_CANONICAL_WITH_COMMENTS; }
+                       | /*EMPTY*/
                         { $$ = XMLSERIALIZE_NO_FORMAT; }

2) This should be added to typedefs.list:
+typedef enum XmlSerializeFormat
+{
+       XMLSERIALIZE_INDENT,                                    /*
pretty-printed xml serialization  */
+       XMLSERIALIZE_CANONICAL,                                 /*
canonical form without xml comments */
+       XMLSERIALIZE_CANONICAL_WITH_COMMENTS,   /* canonical form with
xml comments */
+       XMLSERIALIZE_NO_FORMAT                                  /*
unformatted xml representation */
+} XmlSerializeFormat;

3) This change is not required:
        return result;
+
 #else
        NO_XML_SUPPORT();
        return NULL;

4) This comment body needs slight reformatting:
+       /*
+       * Parse the input according to the xmloption.
+       * XML canonical expects a well-formed XML input, so here in case of
+       * XMLSERIALIZE_CANONICAL or XMLSERIALIZE_CANONICAL_WITH_COMMENTS we
+       * force xml_parse() to parse 'data' as XMLOPTION_DOCUMENT despite
+       * of the XmlOptionType given in 'xmloption_arg'. This enables the
+       * canonicalization of CONTENT fragments if they contain a singly-rooted
+       * XML - xml_parse() will thrown an error otherwise.
+       */

5) Similarly here too:
-                       if (newline == NULL || xmlerrcxt->err_occurred)
+                       * Emit declaration only if the input had one.
Note: some versions of
+                       * xmlSaveToBuffer leak memory if a non-null
encoding argument is
+                       * passed, so don't do that.  We don't want any
encoding conversion
+                       * anyway.
+                       */
+                       if (decl_len == 0)

6) Similarly here too:
+                               /*
+                               * Deal with the case where we have
non-singly-rooted XML.
+                               * libxml's dump functions don't work
well for that without help.
+                               * We build a fake root node that
serves as a container for the
+                               * content nodes, and then iterate over
the nodes.
+                               */

7) Similarly here too:
+                               /*
+                               * We use this node to insert newlines
in the dump.  Note: in at
+                               * least some libxml versions,
xmlNewDocText would not attach the
+                               * node to the document even if we
passed it.  Therefore, manage
+                               * freeing of this node manually, and
pass NULL here to make sure
+                               * there's not a dangling link.
+                               */

8) Should this:
+       * of the XmlOptionType given in 'xmloption_arg'. This enables the
+       * canonicalization of CONTENT fragments if they contain a singly-rooted
+       * XML - xml_parse() will thrown an error otherwise.
Be:
+       * of the XmlOptionType given in 'xmloption_arg'. This enables the
+       * canonicalization of CONTENT fragments if they contain a singly-rooted
+       * XML - xml_parse() will throw an error otherwise.

Regards,
Vignesh