Re: [xslt] libxml2-2.6.7/catalog.c -- various inconsistencies

[Daniel Veillard <veillard redhat com>]

On Wed, Feb 25, 2004 at 10:44:34AM +0100, Peter Breitenlohner wrote:
> Hi Daniel,

  Hi Peter,

> 1. in order to better understand the XML catalog handling I had a look at the
> source code in catalog.c. In doing so I noticed several inconsistencies,
> ranging from (I think) a bug to typos in the embedded docu.
> 
> 1.1 xmlCatalogUnWrapURN l.765
> 	    if ((urn[1] == '2') && (urn[1] == 'B'))
> this can't possibly work and certainly should be
> 	    if ((urn[1] == '2') && (urn[2] == 'B'))
>                                         ^
> same for the other 7 cases

  Dohh, well that tend to prove that nobody uses the URN escaping
that the XML catalog people designed, hardly a surprise to me :-)

> 1.2 xmlCatalogListXMLResolve l.1885
> 	if (pubID == NULL)
> 	    ret = xmlCatalogListXMLResolve(catal, urnID, NULL);
> 	else if (xmlStrEqual(pubID, urnID))
> 	    ret = xmlCatalogListXMLResolve(catal, pubID, NULL);
> 	else {
> 	    ret = xmlCatalogListXMLResolve(catal, pubID, NULL);
> this doesn't make much sense and probably should read
> 	if (pubID == NULL)
> 	    ret = xmlCatalogListXMLResolve(catal, urnID, NULL);
> 	else if (xmlStrEqual(pubID, urnID))
> 	    ret = xmlCatalogListXMLResolve(catal, pubID, NULL);
> 	else {
> 	    ret = xmlCatalogListXMLResolve(catal, pubID, urnID);
>                                                          ^^^^^

   Hum, no, this is just a litteral transcription of the spec in section 7.1.1:

-----------------------------------------
If the system identifier is a URN in the publicid namespace, it is
converted into a public identifier by "unwrapping" the URN. In this case,
one of the following must apply:
   1.  No public identifier was provided. Resolution continues as if the
   public identifier constructed by unwrapping the URN was supplied as
   the original public identifier and no system identifier was provided.
   2.  The normalized public identifier provided is lexically identical
   to the public identifier constructed by unwrapping the URN. Resolution
   continues as if the system identifier had not been supplied.
   3.  The normalized public identifier provided is different from
   the public identifier constructed by unwrapping the URN. This is an
   error. Applications may recover from this error by discarding the
   system identifier and proceeding with the original public identifier.
-----------------------------------------

 Which means that if there is already a Public ID passed, one may just
ignore the System ID if it is different, but it's an error anyway.
The code could be optimized, or an error could be raised, but it's not
wrong. Please consider reading the spec while reviewing the code, you
can't really do the later without knowing the former.

> 1.3 xmlCatalogListXMLResolve l.1902
> 		ret = xmlCatalogXMLResolve(catal->children, pubID, sysID);
> shouldn't this be
> 		ret = xmlCatalogListXMLResolve(catal->children, pubID, sysID);
> ? depends on whether catal->children can be a list of children or is always
> just one child

  I would rather see a process in the other way. Can you exhibit a problem
with the code ? I would expect the catal->children to be a CATALOG i.e. 
without siblings.

> 1.4 xmlACatalogResolve l.2667
>     if (xmlDebugCatalogs) {
>         if (pubID != NULL) {
>             xmlGenericError(xmlGenericErrorContext,
>                             "Resolve: pubID %s\n", pubID);
>         } else {
>             xmlGenericError(xmlGenericErrorContext,
>                             "Resolve: sysID %s\n", sysID);
>         }
>     }
> I think this should be replaced by
>     if (xmlDebugCatalogs) {
>         if ((pubID != NULL) && (sysID != NULL)) {
>             xmlGenericError(xmlGenericErrorContext,
>                             "Resolve: pubID %s sysID %s\n", pubID, sysID);
>         } else if (pubID != NULL) {
>             xmlGenericError(xmlGenericErrorContext,
>                             "Resolve: pubID %s\n", pubID);
>         } else {
>             xmlGenericError(xmlGenericErrorContext,
>                             "Resolve: sysID %s\n", sysID);
>         }
>     }
> or equivalent, because xmlCatalogListXMLResolve and xmlCatalogSGMLResolve
> will resolve either pubID or sysID.

  Well, that's just debugging code. This is used to trace what happen
in debugging mode. Applied but nothing critical.

> 1.5 xmlCatalogLocalResolve l.3436
> ditto

  Applied but nothing critical.

> 1.6 various typos in the embedded docu
> 
> xmlCatalogGetSGMLSystem l.2382
>  * @sysId:  the public ID string
>                 ^^^^^^
>  * Returns the system ID if found or NULL otherwise.
>                ^^^^^^^^^
> what do all these *Resolve routines return? Please clarify

/**
 * xmlCatalogGetSGMLSystem:
 * @catal:  an SGML catalog hash
 * @sysId:  the public ID string
 *
 * Try to lookup the catalog local reference for a system ID
 *
 * Returns the system ID if found or NULL otherwise.
 */

  It's clear that the function takes a public ID and search
a local resource for it.
  
  I changed it to
* Returns the local resource if found or NULL otherwise.

> xmlACatalogResolveSystem l.2579
> xmlCatalogResolveSystem l.3063
> same as above
> 
> xmlCatalogGetSystem l.3499
>  * Try to lookup the system ID associated to a public ID
>                      ?????????                 ^^^^^^^^^
> 
> xmlFreeCatalog l.395
>  * @catal:  a Catalog entry
>                      ^^^^^^
> 
> xmlCatalogDumpEntry l.420
>  * @entry:  the
>                 ^^^^^^^^^
> 
> several places:
> whereas the functions consistently use pubID and sysID, the docu refers to
> them in some places as pubId and sysId.
> 
> 2. I think you should handle the generation of xml2Conf.sh in the same way
> as xsltConf.sh, i.e. via a sed script in the Makefile instead of configure
> substitutions. Otherwise the resulting xml2Conf.sh may refer to unexpanded
> variables (e.g. XML2_LIBDIR="-L${libdir}")

  Can you send a patch for the remaining issues. Use diff -c to generate
a contextual patch.

  Thanks,

Daniel

-- 
Daniel Veillard      | Red Hat Network https://rhn.redhat.com/
veillard@redhat.com  | libxml GNOME XML XSLT toolkit  http://xmlsoft.org/
http://veillard.com/ | Rpmfind RPM search engine http://rpmfind.net/