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

[Andrew Tridgell]

Simo,

A few comments on schema.c:

 - what is attrsyn[] for ? it seems to be unused

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

   if (ret == 1) {
	/* lots and lots of code */
   } else {
	return -1;
   }

   that means all that code is indented unnecessarily, which makes it
   harder to read. Do one of the following:

   if (ret != 1) {
       return -1;
   } 
   /* rest of code here */

   or this:

   if (ret != 1) goto failed;
   /* rest of code here */

   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.

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

Cheers, Tridge