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

[Ben Greear]

On 03/10/2011 08:02 AM, Steven Simpson wrote:
> 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...

'stg' can help edit existing patch series.

To combine, I often save the individual patches with format-patch,
reset the tree to before the patches were applied, and manually
apply them with patch -p1 < ...
and then git commit the resulting thing.

One suggestion:  Be sure to save the patches clear of the tree
before you go start messing with git reset, though normally you
can recover from pretty much anything you do in git...

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

There may be places using 0, but I vastly prefer NULL for pointers,
and would be happy to accept any patches that removed usage of '0' for
pointers.

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

Those are fine with me.

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

Probably best to disable OLSR instead.  I don't think anyone actually
uses it, so I didn't bother to fix the disable_profile problems
in it.

Thanks,
Ben


-- 
Ben Greear <greearb at candelatech.com>
Candela Technologies Inc  http://www.candelatech.com