[PATCH] s4: initial code for the topology algorithm

[Crístian Viana]

Thanks for your sugestions, metze!

 - we avoid the ugly (*g)-> notation


I created some temporary variables (like "ge" on the previous code) to avoid
that ugly (and sometimes long) notation.

 - we use a better name 'g' => 'graph'


I was trying to use the same names as in the original algorithm, but longer
names instead of single letters is actually better.

 - TODO: you should avoid pointers to GUID and copy them


I think I wasn't using any pointers to GUID, the only case is the "data"
field for the list structure.

 Maybe ../librpc/idl/drsblobs.idl


I'll look at this file more carefully sooner, I don't understand IDL yet.
For now, I'll leave the constants as #define macroes, but I'll change them.

Thanks again for your suggestions, I'll send other patches soon.

Cheers,

On Wed, Feb 10, 2010 at 10:21 AM, Stefan (metze) Metzmacher <metze at samba.org
> wrote:

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


-- 
Crístian Deives dos Santos Viana [aka CD1]
Sent from Campinas, SP, Brazil
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-make-code-more-readable.patch
Type: text/x-patch
Size: 27062 bytes
Desc: not available
URL: <http://lists.samba.org/pipermail/samba-technical/attachments/20100212/ed246d64/attachment.bin>