[oe] [OE] [PATCH meta-networking] quagga/ripd: Fix two bugs after received SIGHUP signal
Joe MacDonald
joe at deserted.net
Fri Nov 1 14:29:20 UTC 2013
And to close the loop, I merged the modified version of your patch that
we discussed below. Thanks.
-J.
[Re: [oe] [OE] [PATCH meta-networking] quagga/ripd: Fix two bugs after received SIGHUP signal] On 13.10.31 (Thu 09:59) Joe MacDonald wrote:
> [Re: [oe] [OE] [PATCH meta-networking] quagga/ripd: Fix two bugs after received SIGHUP signal] On 13.10.31 (Thu 09:52) Xufeng Zhang wrote:
>
> > On 10/30/2013 09:36 PM, Joe MacDonald wrote:
> > >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?
> >
> > I think it's fine.
> > Do I need to send V2 patch?
>
> Nope, if you're happy with my proposed amendments, I'll just update the
> patch in my tree.
>
> > >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.
> >
> > Ok, I'll do this job for the other daemons.
>
> Fantastic, thanks!
>
> -J.
>
> > Thanks a lot for the review!
> >
> >
> >
> > Thanks,
> > Xufeng
> >
> >
> > >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/20131101/6ee19bba/attachment-0002.sig>
More information about the Openembedded-devel
mailing list