Donald,
"cannot find a nexthop via pim_nexthop_lookup" is consistent with
the our observation that I explained in my previous email. I agree with
you that the assertion is correct. My main point was whether we should
just avoid getting into that situation. I had in mind that this might be
done lower in the stack. So thanks for providing the code fix. I will
let you know if your it works or not. I'd probably add code to log that
also, because normally we want to know if pimd fails to find a nexthop.
Thanks,
Jafar
On 6/12/2015 9:25 AM, Donald Sharp wrote:
If I am reading the code correctly, pim_joinprune_send is called with
the interface pointer from the 'struct pim_upstream'. This data
structure is setup in pim_upstream_new, in which if it cannot find a
nexthop via pim_nexthop_lookup it will not set the pointer for the
interface if it cannot find a nexthop interface. The error code is
ignored in pim_upstream_new when that fails.
I would think leaving the assert in place is the correct thing to do
(since we should never get to a pim_joinprune_send with a NULL
interface pointer), but to have something like this:
diff --git a/pimd/pim_upstream.c b/pimd/pim_upstream.c
index d02f915..a4d274a 100644
--- a/pimd/pim_upstream.c
+++ b/pimd/pim_upstream.c
@@ -349,6 +349,7 @@ static struct pim_upstream
*pim_upstream_new(struct in_addr source_addr,
struct in_addr group_addr)
{
struct pim_upstream *up;
+ enum pim_rpf_result rpf_result;
up = XMALLOC(MTYPE_PIM_UPSTREAM, sizeof(*up));
if (!up) {
@@ -372,7 +373,11 @@ static struct pim_upstream
*pim_upstream_new(struct in_addr source_addr,
up->rpf.source_nexthop.mrib_route_metric =
qpim_infinite_assert_metric.route_metric;
up->rpf.rpf_addr.s_addr = PIM_NET_INADDR_ANY;
- pim_rpf_update(up, 0);
+ rpf_result = pim_rpf_update(up, 0);
+ if (rpf_result == PIM_RPF_FAILURE) {
+ XFREE(MTYPE_PIM_UPSTREAM, up);
+ return NULL;
+ }
listnode_add(qpim_upstream_list, up);
Please note I have not done any actual testing of this code. It was
just the only place in the code where we don't set the interface
pointer. I have no clue what the callers of pim_upstream_new do,yet,
when they receive a NULL return for 'struct pim_upstream *'. More
work would need to be done to verify this.
donald
On Fri, Jun 12, 2015 at 12:51 AM, Jafar Al-Gharaibeh <[email protected]
<mailto:[email protected]>> wrote:
Donald,
We have been using pimd for a couple of years and we had
submitted several bug fixes that seem to have made it to the
official sources which is great, especially with the addition of
pimd to the official Quagga. There are several places in the code
where asserts appear to be an "overkill". In at least two
locations, we replaced asserts with warnings and bail out of the
function where the condition that triggers the assert occurs. pimd
just moves on afterward instead of terminating when the error
shouldn't have been fatal to begin with I think. Here is an example:
diff --git a/pimd/pim_join.c b/pimd/pim_join.c
index 9d8e001..93ec09f 100644
--- a/pimd/pim_join.c
+++ b/pimd/pim_join.c
@@ -301,6 +301,21 @@ int pim_joinprune_send(struct interface *ifp,
int pim_msg_size;
int remain;
+ /* TODO: should we die or bail out ? */
+ if (!ifp) {
+ char source_str[100];
+ char group_str[100];
+ char dst_str[100];
+ pim_inet4_dump("<src?>", source_addr, source_str,
sizeof(source_str));
+ pim_inet4_dump("<grp?>", group_addr, group_str,
sizeof(group_str));
+ pim_inet4_dump("<dst?>", upstream_addr, dst_str,
sizeof(dst_str));
+ zlog_warn("%s: no interface for pim %s(S,G)=(%s,%s) to
upstream=%s",
+ __PRETTY_FUNCTION__,
+ send_join ? "Join": "Prune",
+ source_str, group_str, dst_str);
+ return 0;
+ }
+
zassert(ifp);
pim_ifp = ifp->info;
Does this make sense, or is this a terrible idea?! I can bring up
at least another example. The main goal is to determine if these
are really fatal or recoverable errors.
Thanks,
Jafar
_______________________________________________
Quagga-dev mailing list
[email protected]
https://lists.quagga.net/mailman/listinfo/quagga-dev
_______________________________________________
Quagga-dev mailing list
[email protected]
https://lists.quagga.net/mailman/listinfo/quagga-dev
