[oe] [OE] [PATCH meta-networking] quagga/ripd: Fix two bugs after received SIGHUP signal
Xufeng Zhang
xufeng.zhang at windriver.com
Thu Oct 31 01:52:08 UTC 2013
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?
> 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.
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
>>>>
>>
>
More information about the Openembedded-devel
mailing list