svn commit: samba r4785 - in branches/SAMBA_4_0/source: . script
idra at samba.org
Mon Jan 17 16:24:43 GMT 2005
On Mon, 2005-01-17 at 23:51 +1100, Andrew Tridgell wrote:
> A few comments on schema.c:
> - what is attrsyn for ? it seems to be unused
Yes it is unused, it was there for reference as I still do not check
attributes respect the defined syntax.
> - when checking for allocation failure, please test for NULL not 0. I
> keep thinking its an integer :-)
> - please keep the mainline code simpler, and treat errors as
> exceptions. For example, in get_object_objectclasses() you do this:
ok, I'll fix it asap
> The aim is to keep the "main line" code clear and clean, rather
> than making the "main line" code look like an exception.
> - one or two comments would be handy explaining what a function does
> when it isn't obvious. For example, it isn't clear to me what
> "get_check_list()" does.
I'll add a comment to get_check_list (the comment is into
schema_add_record() body actually)
> Even with the above changes, I'm still concerned about the depth of
> nesting of all those for loops. I don't really understand what they
> all do yet, but I'm pretty sure some utility functions along the lines
> of ldb_msg_find_element() would be a great help to readability.
I will try to modularize the code some more, but I do not see much space
> I also wonder why you are copying strings everywhere. Maybe there is a
> reason, but it would seem that just pointer assignments could replace
> a lot of the calls to talloc_strdup(). That would be much faster and
> would be much less code (as no error checking is needed).
I think I can do a lot of talloc_steal() or talloc_reference() now that
we moved ldb to talloc, that code has not changed when I made the
interim conversion to talloc.
The strdup where necessary as I search all the attributes for all the
schema referenced by the current operation and need to keep them around
while searching the tree (1-2 search for each objectclass involved, no
Simo Sorce - idra at samba.org
Samba Team - http://www.samba.org
Italian Site - http://samba.xsec.it
More information about the samba-technical