Re: [xml] modifying tree and attributes

[Rob Richards <rrichards ctindustries net>]

Daniel Veillard wrote:

> On Sun, Oct 23, 2005 at 08:16:31AM -0400, Rob Richards wrote:
>  
>
> > Where is the line drawn on what is expected the caller to handle and 
> > what the lib handles?
> >    
>  I would say the library should be made as resilient as possible.
>
> Looking at the bug, xmlAddChild() code and xmlHasNsProp() it seems
> to me that xmlHasProp() should never be called and 
>    lastattr = xmlHasNsProp(parent, cur->name, NULL);
> should be used instead if cur->ns is NULL. bug #319108 should be just
> fixed that way IMHO.
>  
Technically I would agree about that, API just seems odd having 
xmlGetNoNsProp but not xmlHasNoNsProp.
Don't think its necessary to cleanup pointers? If this is the case then 
xmlUnsetProp doesnt need to do so either then correct?
As long as API is used I dont see a problem with the pointers, but a 
fringe case where someone tries to access ->prev of the first attribute 
would get garbage. (this would happen in cases where the first attribute 
is freed - found in many instances in the API. the element's 
->properties get set to the next attribute, but this attribute's ->prev 
never gets set to NULL so now pointing to free'd memory)

> > Looking at a few other functions:
> > xmlNewXXXProp - imo up to caller to handle checks as code does no checks 
> > for anything so no issues here.
> >    
>
>  Adding such DTD related checks in xmlNewPropInternal() and trying to
> channel all modification to attributes on that routine sounds the right
> approach to me (i.e. only one copy of the code, as complex and strict as
> need to be).
>  
>
> > xmlSetXXXProp - grey area here. function checks for exsiting attribute 
> > and IDs but not XML_ATTRIBUTE_DECL (really only #FIXED would cause any 
> > problems). But in the case ofsetting a prop with ns, it is up to the 
> > caller to insure ns is in scope. so basically some things handled here 
> > while others arent.
> >    
>
>  I would not check the scopedness of the attribute passed there (it may
> not yet have been added to the element), but I would check the type of the
> attribute to avoid breaking #FIXED, and potentially other checks which may
> be needed.
>  
The only concern I had about adding stuff to the xmlNew.. functions is 
now its adding more overhead (i.e. making it slower) by having to do 
checks even if the callee knows they dont need to be done. I consider 
this a lower level function and xmlSetxxxProp higher level, so would 
have done checking when calling the set functions (i.e. have it call the 
get prop functions - which also would return any DTD info and work with 
any returned attribute/decl; otherwise when nothing is returned, call 
the new prop functions since its safe not to do checks).

An example of what I mean could be found when SAX2 is building a tree. 
It already does checks (no dupe attributes), so the checks would be 
duplicated when xmlNewNsProp is called. Checking #FIXED here as well 
means that even if not validating it would be enforced (which gets me 
thinking about wether trying to enforce #FIXED in the first place is 
really a good idea or should only happen when validating the document). 
Myself, in certain places I do checks prior before as well, as the 
attribute needs to be unlinked anyways and I know there is no DTD. I 
dont want to use my personal example as an argument here as it doesnt 
really matter to me having the checks. I just wanted to make sure it's 
understood that making the API more resillient everywhere will have an 
impact on speed (which is one thing this library excels at) and not just 
affect those answering this email.

>  I take patches :-)
>  
Yea, working on it :)

And Yes, I am fully aware I am going off in many directions here at 
once, but looking at one thing leads to another and so on...

Rob