[oe] [OE] [PATCH meta-networking] quagga/ripd: Fix two bugs after received SIGHUP signal
Joe MacDonald
joe at deserted.net
Wed Oct 30 13:36:17 UTC 2013
Hi Xufeng,
[Re: [oe] [OE] [PATCH meta-networking] quagga/ripd: Fix two bugs after received SIGHUP signal] On 13.10.28 (Mon 11:37) Xufeng Zhang wrote:
> On 10/25/2013 09:31 PM, Joe MacDonald wrote:
> >[Re: [oe] [OE] [PATCH meta-networking] quagga/ripd: Fix two bugs after received SIGHUP signal] On 13.10.25 (Fri 09:47) Xufeng Zhang wrote:
> >
> >>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.
> >I'm not sure that's the interpretation I would have taken from the
> >quoted text, but if what you indicate is what the maintainer meant, then
> >is he not saying that the SIGHUP handling issue also exists for other
> >routing daemons in the suite?
> quoted the sentence:
> "
>
> 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.
>
> "
> All the other daemons also refer to the ifp->connected list, and I
> have checked the SIGHUP handler code of all daemons,
> except ospfd and ospf6d(they only print a log when SIGHUP is
> received), all the other daemons suffer the same problem as ripd
> since
> they reload the configuration file.
>
> >Are you saying a similar patch would then
> >be required for potentially all daemons in quagga?
> Not all.
Six of eight, then. ;-) Nine, if watchquagga also contains dodgy
SIGHUP handler code, but it probably doesn't care about the connected
list.
> >Don't get me wrong, repairing one is better than repairing none, but on
> >my first read of his comments it didn't seem like the maintainer even
> >agreed that there was a bug at all and your interpretation suggests that
> >the same bug likely impacts all quagga daemons. Any sense on whether
> >that's the case?
> "
>
> 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.
>
> "
> My understanding is that if there is a bug related to
> ifp->connected, we should not fix in such specific way.
> Yes, the maintainer didn't agree that there is really a bug here,
> but this is really a abnormal behavior:
> every time when SIGHUP is received, the same address is appended to
> ifp->connected list over and over again,
> so if too much SIGHUP are received, there will be a flood for the
> advertised packets.
Okay, so given that, I'm inclined to merge this set with a note in the
upstream-status in the patch indicating it is unlikely to be merged.
How about "Submitted" with a footnote pointing at the discussion thread?
Do you think it'd be possible to provide similar patches for the other
daemons that suffer this problem? I'll take the current submission, but
if you had time to do a follow-up later on for the other daemons, that'd
be great.
Thanks,
-J.
>
>
> Thanks,
> Xufeng
>
> >-J.
> >
> >>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 at 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 at 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 at lists.openembedded.org
> >>>>http://lists.openembedded.org/mailman/listinfo/openembedded-devel
> >>_______________________________________________
> >>Openembedded-devel mailing list
> >>Openembedded-devel at lists.openembedded.org
> >>http://lists.openembedded.org/mailman/listinfo/openembedded-devel
>
--
-Joe MacDonald.
:wq
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 198 bytes
Desc: Digital signature
URL: <http://lists.openembedded.org/pipermail/openembedded-devel/attachments/20131030/2db24bfa/attachment-0002.sig>
More information about the Openembedded-devel
mailing list