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