[PATCH] s4: initial code for the topology algorithm

[Stefan (metze) Metzmacher]

Crístian Viana schrieb:
> Hi everyone,
> 
> I started to write the code to compute the DC topology in an AD domain,
> based on the Microsoft documentation (MS-ADTS 7.2.2.3.4). The first patch is
> attached and I'd like they get reviewed.
> 
> The algorithm is rather large, so I created a wiki page with a more
> simplified version of the original document (
> http://wiki.samba.org/index.php/DC_topology). This first patch has only the
> initial setup functions so I thought it'd be small, but it quickly grew to
> hundreds of lines of code.
> 
> I have some question/observations regarding this patch:
> 
> - I created a structure for a GUID list but I think it can be useful outside
> this code and it needs a proper header file to be declared on.

- Move all stuff from kcc_topology.h to kcc_topology.c for now,
  it's only used there.

- Then make almost every function static

- prefix every not only some functions and structures with kcctpl_

- you don't need cmp_guid(), do a 'git grep QSORT_CAST' to see
  examples how to avoid it.

- Please make the code a bit more readable, that will help others (but
  also yourself) to review and understand the code. This will also
  discover some real bugs.

As an example I modify one of your functions:

static NTSTATUS kcctpl_create_graph(TALLOC_CTX *mem_ctx,
				    struct kcctpl_guid_list *guids,
				    struct kcctpl_graph **_graph)
{
	struct kcctpl_graph *graph;
	unsigned int i;

	graph = talloc_zero(mem_ctx, struct kcctpl_graph);
	NT_STATUS_HAVE_NO_MEMORY(graph);

	graph->vertices.count = guids->count;
	graph->vertices.data = talloc_zero_array(graph,
						 struct kcctpl_vertex,
						 guids->count);
	NT_STATUS_HAVE_NO_MEMORY_AND_FREE(graph->vertices.data, graph);

	qsort(guids->data, guids->count, sizeof(struct GUID),
	      QSORT_CAST GUID_compare);

	for (i = 0; i < guids->count; i++) {
		graph->vertices.data[i].id = guids->data[i];
	}

	*_graph = graph;
	return NT_STATUS_OK;
}

Note:
 - the callers *_graph variable is only modified on success.
 - we avoid the ugly (*g)-> notation
 - we use a better name 'g' => 'graph'
 - each logical step is delimited by an empty line.
 - graph->vertices.data is a talloc child of graph
   and we now cleanup graph if the allocation of graph->vertices.data
   fails.
 - TODO: you should avoid pointers to GUID and copy them

> - Where do I define some constants, like NTDSTRANSPORT_OPT_BRIDGES_REQUIRED?
> Is there a central file for that?

Maybe ../librpc/idl/drsblobs.idl

> - Some attributes (like siteLink) may have multiple values and I need to
> search for all of them. Is the right way to do that
> with ldb_msg_find_element?
Should be and then enumeration over all element values.

> - To get the attribute "schedule" from an ldb_search, I need to convert an
> ldb_val to an int[]. I once asked in this mailing list how to do the
> opposite and I learned that it's using the function data_blob_const. How do
> I convert it the other way around? I'm still not familiar with NDR and
> blobs.

I'll let tridge explain that (I'm too busy currently, sorry!)

int schedule[84] is wrong! shouldn't it be uint8_t schedule[84]?

Most times it's better to use [u]int[8|16|32|64]_t as type instead
of just int.

> - In one function I need to perform multiple ldb_search'es with different
> attributes. As string array handling in C isn't so nice, I decided to create
> a single array having the maximum size I'll need with multiple NULL values
> and it's filled as needed. I don't know if that's the best approach or if it
> even works for ldb_search, but that was the best I could think while saving
> memory and code.

Just define multiple static const char * const *foo_attr variables
and use them as needed.

> - The original Microsoft documentation mentions the constant
> NTDSSETTINGS_OPT_W2K3_BRIDGES_REQUIRED, but I can't find any reference to it
> in MSDN. When I search for that term, the single occurrence is this
> documentation I'm reading. Google can find only 3 ocurrences: one being the
> MS-ADTS documentation and the other two are some third-party codes which use
> that constant. How can I find out more about this field? I don't even know
> its value. The third-party codes I found on Google use the value (1 << 12)
> for that variable, but I don't know if I can trust it. Should I e-mail
> Microsoft asking them?
> - I see there are functions to manipulate linked lists (dlinklist.h) but I
> missed the functions to manipulate array lists, I repeated too much code
> doing that. Aren't array lists a common technique in Samba?
> 
> I haven't run any of those functions yet so they may have obvious errors,
> but that'll be the next thing I'll do tomorrow. By the way, I was wondering
> how I'm going to make sure this code is working the way it should before it
> gets completely finished.

Some unit tests would be great, which tests against predefined values.

Or a test that searches on a remote server, calculate the servers
topology and compares it to what the server has calculated.
I think we need that anyway, because we can't blindly rely on the
documentation.

metze

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 260 bytes
Desc: OpenPGP digital signature
URL: <http://lists.samba.org/pipermail/samba-technical/attachments/20100210/a1199848/attachment.pgp>