[quagga-dev 11509] Re: [PATCH] pimd merge

Everton Marques everton.marques at gmail.com
Thu Sep 18 19:14:02 BST 2014


Paul,

Thanks for the comments. Replies inline.

On Wed, Sep 17, 2014 at 10:49 AM, Paul Jakma <paul at jakma.org> wrote:

>
> Just curious if there's a need to replicate the inet_checksum in pim_utils?
>

I don't think there is need.
I believe the pim_util's version of checksum just predates my knowledge
about the one provided by Quagga.
pim_util's checksum compilation is disabled for a long time.


> Also, some commits have conflict markers still in them. E.g. zebra.h in
> "[pim] Initial pim 0.155"; vtysh.c in "[pim] pim commands added to vtysh";
> The zebra.h is resolved in a latter commit, but the vtysh.c one doesn't
> seem to be.
>

True, good catch.


> The WHY_SSM file: I'd probably keep this document free-standing, and avoid
> linking to other pages.
>

Agreed. I added the link just because its content is very enlightening.

Next in "pimd: fix wtf code", why should the received distance be ignored
> in the pim_zebra.c change? A comment in the code might be useful there?
>

I see what you mean, because I just got confused over it again, but the
distance is not ignored, and it seems fine as is:

   api.distance = CHECK_FLAG(api.message, ZAPI_MESSAGE_DISTANCE) ?
-    api.distance = stream_getc(s) :
+    stream_getc(s) :
     0;


> The "[pim] Version up to 0.161[pim] Version up to 0.161" commit removes a
> static from lib/command.c default_motd which seems needless (at least in
> that commit?)- just a random change that shouldn't be there?
>

Yes, probably a quick fix for a compilation warning.
default_motd is changed in order to display pimd version.
But I think the best fix is actually to remove the motd tweaking
altogether, because pimd should not have its own version once included in
Quagga.


>
> The "pimd: Prevent interfaces' addresses duplication when zebra connection
> is restored" patch, is this fix actually generally applicable to client
> connection restoration, i.e. should it be in libzebra code? How transparent
> is this to the clients? Basically, I'd like to a read bit more about what's
> going on and what's expected of clients (especially, what if any
> expectations have changed). The state machines around interface handling
> are not the simplest, so docs on changes here would really help.
>

Whenever zclient reconnects to zserv, the lib code automatically re-adds
all interfaces' addresses to lib's data structures.
It's burdensome for a daemon to circumvent such interface address
duplication.
The change you pointed out has 2 effects:
1) Provide an optional new function pointer callback
(zclient.zclient_broken) to notify the application about the connection
loss. Enabling the daemon to easily handle such event. Because it is a new
optional pointer, initialized to 0 by default, it should not affect
existing daemons.
2) Clear all interfaces' addresses on connection loss, to avoid address
duplication, because on reconnection the zclient lib code will re-assign
them.
It's expected that daemons wanting to survive reconnection to zebra daemon
could set zclient.zclient_broken to a callback handler. Otherwise, nothing
is expected.


>
> The remaining comments are "would be nice to have" should there be a need
> to rebase:
>
> "pimd: fix wtf code": the #if 0'ed code in pim_time, that gets updated by
> a next commit to depend on a define instead. If you're rebasing before
> merge, could you squash in that next commit? Actually, on what platforms
> would gettimeofday() not be available? Do we care about such platforms
> enough to add the pain that optional/fallback code can cause?
>

Sorry, I can't remember the reason for this gettime() mess.
I suppose the code could be simplified to rely solely on gettimeofday().


>
> Some of the "pimd: cleanup" commits are not really pimd cleanups, but,
> e.g., of zebra RIB code. The subject line could be a bit more accurate,
> e.g. "zebra: cleanups to zebra_rib". Speaking of which there are 2 in a row
> that are on zebra_rib that could be squashed together, should you happen to
> rebase again. :)
>
> There is 1 "pimd: cleanup" that really is a mish-mash of commits, to
> pimd/COMMANDS, zebra/*rib.{c,h} and zebra/zebra_vty.c that really should be
> split up into separate patches and perhaps given slightly more informative
> subjects, if there's a rebase.
>
> Ditto on squashing on "pimd: Export zclient_socket_un()." - 2 together,
> same stuff.


Ok, will do.

Soon I'm pushing updates addressing the issues.

Cheers,
Everton
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.quagga.net/pipermail/quagga-dev/attachments/20140918/63fc1b3d/attachment-0001.html>


More information about the Quagga-dev mailing list