svn commit: samba r4785 - in branches/SAMBA_4_0/source: . script

[Simo Sorce]

On Mon, 2005-01-17 at 23:51 +1100, Andrew Tridgell wrote:
> Simo,
> 
> 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 :-)

oops :)

>  - 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.

right

>  - 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
for that.

> 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
optimizations yet)

Simo.

-- 
Simo Sorce    -  idra at samba.org
Samba Team    -  http://www.samba.org
Italian Site  -  http://samba.xsec.it