Re: [PATCH meta-networking] quagga/ripd: Fix two bugs after received SIGHUP signal

[Xufeng Zhang <xufeng.zhang@windriver.com>]

On 10/25/2013 01:44 AM, Joe MacDonald wrote:
> [[oe] [OE] [PATCH meta-networking] quagga/ripd: Fix two bugs after received SIGHUP signal] On 13.10.24 (Thu 17:48) Xufeng Zhang wrote:
>
>    
>> There are two problems for ripd implementation after received
>> SIGHUP signal:
>> 1). ripd didn't clean up ifp->connected list before reload
>>      configuration file which makes the same advertise packet
>>      being sent multiple times(depends on how many SIGHUP was recieved).
>> 2). ripd reset ri->split_horizon flag to RIP_NO_SPLIT_HORIZON
>>      during restart which is different from the flag when ripd is
>>      firstly started up, leading to unnecessary route to be advertised.
>>
>> [YOCTO #5266]
>>      
> Hey Xufeng,
>
> Normally I wouldn't go this deep into the detail but since you
> referenced the Yocto bug and the Quagga discussion in the commit log and
> the bug itself, I thought I'd try to understand what's going on here.
> The last thing I saw from the Quagga maintainer is this:
>
>     The ifp->connected list contains connected prefixes of a given
>     interface. These exist regardless of particular routing protocols and
>     emptying the list during ripd reconfiguration would be plain wrong. I
>     allow for a chance there is a bug that is related to ifp->connected
>     and SIGHUP handling at once, but not in this specific way.
>    
What he said is that this ifp->connected list is exist in all routing 
protocols,
and if it needs cleanup, it should be done in a more general way, such as
in zebra client which is independent of routing protocol.
But I have specified the reasons in the above email that why it's not 
easy to
implement this general way:
#########################################
More comments:
Generally this connected interface address stuff is cleaned up in 
rip_interface_address_delete()
which is triggered by zsend_interface_address(), but just as the 
annotation said above
zsend_interface_address() function in zebra/zserv.c, 
ZEBRA_INTERFACE_ADDRESS_DELETE
message could only happens in two cases:
1). ip address uninstall
2). RTM_NEWADDR on routing/netlink socket

It didn't take the SIGHUP signal into consideration, but since the 
SIGHUP is handled
by every daemon independently, this cleanup is not easy to be 
implemented commonly,
so I think this is a proper way to do this.
#########################################
So it needs to redesign a lot of stuff to implement this.

> and it looks like the patch here is the same as the one that the
> maintainer indicated is "plain wrong".
>   Can you help me get some
> confidence that this isn't going to have negative side-effects if we
> integrate it?
>    
Yes, there should be no side-effects:
1. This modification is only going into ripd daemon and it only takes 
effect when ripd is restarting.
2. The fix for problem 1). just remove the previously connected 
addresses of the interface, and these
     connected addresses list will be reconstructed when ripd read the 
configuration during restarting.
3. The fix for problem 2). definitely has not any side-effect because I 
just restore the horizon flag to
     the values when ripd daemon is firstly start up.

And the customer has already test the fix for a lot of times, no problem 
is found.


Thanks,
Xufeng

> -J.
>
>    
>> Signed-off-by: Xufeng Zhang<xufeng.zhang@windriver.com>
>> ---
>>   .../ripd-fix-two-bugs-after-received-SIGHUP.patch  |   49 ++++++++++++++++++++
>>   .../recipes-protocols/quagga/quagga.inc            |    3 +-
>>   2 files changed, 51 insertions(+), 1 deletions(-)
>>   create mode 100644 meta-networking/recipes-protocols/quagga/files/ripd-fix-two-bugs-after-received-SIGHUP.patch
>>
>> diff --git a/meta-networking/recipes-protocols/quagga/files/ripd-fix-two-bugs-after-received-SIGHUP.patch b/meta-networking/recipes-protocols/quagga/files/ripd-fix-two-bugs-after-received-SIGHUP.patch
>> new file mode 100644
>> index 0000000..c081143
>> --- /dev/null
>> +++ b/meta-networking/recipes-protocols/quagga/files/ripd-fix-two-bugs-after-received-SIGHUP.patch
>> @@ -0,0 +1,49 @@
>> +ripd: Fix two bugs after received SIGHUP signal
>> +
>> +There are two problems for ripd implementation after received
>> +SIGHUP signal:
>> +1). ripd didn't clean up ifp->connected list before reload
>> +    configuration file.
>> +2). ripd reset ri->split_horizon flag to RIP_NO_SPLIT_HORIZON
>> +    which lead to the unnecessary route to be advertised.
>> +
>> +Upstream-Status: Pending
>> +
>> +Signed-off-by: Xufeng Zhang<xufeng.zhang@windriver.com>
>> +---
>> +--- a/ripd/rip_interface.c
>> ++++ b/ripd/rip_interface.c
>> +@@ -500,6 +500,8 @@
>> +   struct listnode *node;
>> +   struct interface *ifp;
>> +   struct rip_interface *ri;
>> ++  struct connected *ifc;
>> ++  struct listnode *conn_node, *next;
>> +
>> +   for (ALL_LIST_ELEMENTS_RO (iflist, node, ifp))
>> +     {
>> +@@ -514,6 +516,13 @@
>> + 	  thread_cancel (ri->t_wakeup);
>> + 	  ri->t_wakeup = NULL;
>> + 	}
>> ++
>> ++      for (conn_node = listhead (ifp->connected); conn_node; conn_node = next)
>> ++        {
>> ++          ifc = listgetdata (conn_node);
>> ++          next = conn_node->next;
>> ++          listnode_delete (ifp->connected, ifc);
>> ++        }
>> +     }
>> + }
>> +
>> +@@ -548,8 +557,8 @@
>> + 	  ri->key_chain = NULL;
>> + 	}
>> +
>> +-      ri->split_horizon = RIP_NO_SPLIT_HORIZON;
>> +-      ri->split_horizon_default = RIP_NO_SPLIT_HORIZON;
>> ++      ri->split_horizon = RIP_SPLIT_HORIZON;
>> ++      ri->split_horizon_default = RIP_SPLIT_HORIZON;
>> +
>> +       ri->list[RIP_FILTER_IN] = NULL;
>> +       ri->list[RIP_FILTER_OUT] = NULL;
>> diff --git a/meta-networking/recipes-protocols/quagga/quagga.inc b/meta-networking/recipes-protocols/quagga/quagga.inc
>> index 89b9f7a..bf37067 100644
>> --- a/meta-networking/recipes-protocols/quagga/quagga.inc
>> +++ b/meta-networking/recipes-protocols/quagga/quagga.inc
>> @@ -31,7 +31,8 @@ SRC_URI = "http://download.savannah.gnu.org/releases/quagga${QUAGGASUBDIR}/quagg
>>              file://quagga.default \
>>              file://watchquagga.init \
>>              file://watchquagga.default \
>> -           file://volatiles.03_quagga"
>> +           file://volatiles.03_quagga \
>> +           file://ripd-fix-two-bugs-after-received-SIGHUP.patch"
>>
>>   PACKAGECONFIG ??= ""
>>   PACKAGECONFIG[cap] = "--enable-capabilities,--disable-capabilities,libcap"
>>      
>>
>>
>> _______________________________________________
>> Openembedded-devel mailing list
>> Openembedded-devel@lists.openembedded.org
>> http://lists.openembedded.org/mailman/listinfo/openembedded-devel
>>