[Xorp-hackers] Using results from one XRL to respond to another

[Steven Simpson]

Hi Ben,

On 08/03/11 18:07, Ben Greear wrote:
> First:  It seems at least some of the 3rd patch is changing code from the
> first or second patch.  This increases the amount of code review needed,
> so I'm hoping you can re-work this so that the third patch is merged
> into the previous patch(es).  You are welcome to have more smaller
> patches,
> just don't have a later patch do changes to the previous ones if
> possible.

I just sent the output of 'git format-patch'; thought it might have
generated just one patch, but it didn't and I couldn't find an option to
collapse them.  I'd rather have sent just one anyway - they're not
functional individually, just compilable.  I now seem to have managed to
produce a single patch with all changes, though that may be redundant in
light of the potential #ifdef requirement...

> Second:  I *think* you are using '0' when you are passing a NULL pointer.
> Please use 'NULL' instead so it's obvious it's a pointer type and not an
> integer value.  I know that they are in practice equivalent on any modern
> architecture...

Hmm, I suspect I saw 0 being used somewhere already (in
FinderMessengerBase::dispatch_xrl?), and assumed it was the local
convention.  I will use NULL, however, as you request.

> Third:  Please compare the size of the stripped binaries before and after
> your changes.  If your changes cause a significant increase in disk space
> usage, we may want to allow them to be #ifdef'd out:  There are folks who
> are trying to use xorp with very small storage systems.  You can check
> the SConstruct file for examples of how to compile out things (see
> 'disable_profile', etc).

I think I counted about 2MB increase stripped and using
disable_warninglogs=True disable_tracelogs=True disable_fatallogs=True
disable_infologs=True disable_assertlogs=True disable_errorlogs=True
disable_otherlogs=True disable_profile=True, and 7MB unstripped.  I
expect it's all the inlined template instantiations for callbacks...?

It sounds like a lot, so I'm having a go at an #ifdef'd version. 
XORP_ENABLE_ASYNC_SERVER is the macro name, and enable_async_server is
the scons argument.  Let me know if you prefer other names.

Btw, disable_profile=True also raised an error in olsr4.tgt, so I had to
temporarily remove profile/0.1 to get beyond it.

> Fourth:  Please add more verbose descriptions to the individual patches.

Will do.

> And finally:  I'd like a:
>
> Signed-off-by: [you] <your-email>

Oh, so that's what that's for!  No problem.

Cheers,

Steven

--