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

Joe MacDonald joe at deserted.net
Thu Oct 31 13:59:12 UTC 2013 

[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/20131031/aef92363/attachment-0002.sig>

More information about the Openembedded-devel mailing list