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

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

[Xufeng Zhang]

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]

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"
-- 
1.7.0.2

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

[Xufeng Zhang]

On 10/24/2013 05:48 PM, 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).
>    

More comment:
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.


Thanks,
Xufeng

> 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]
>
> 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"
>    

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

[Joe MacDonald]

[-- Attachment #1: Type: text/plain, Size: 4662 bytes --]

[[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.

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?

-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"
-- 
-Joe MacDonald.
:wq

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 205 bytes --]

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

[Xufeng Zhang]

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
>>      

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

[Joe MacDonald]

[-- Attachment #1: Type: text/plain, Size: 7972 bytes --]

[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?  Are you saying a similar patch would then
be required for potentially all daemons in quagga?

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?

-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@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
> 
> _______________________________________________
> Openembedded-devel mailing list
> Openembedded-devel@lists.openembedded.org
> http://lists.openembedded.org/mailman/listinfo/openembedded-devel

-- 
-Joe MacDonald.
:wq

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 205 bytes --]

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

[Xufeng Zhang]

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.

> 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.


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@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
>>>>          
>> _______________________________________________
>> Openembedded-devel mailing list
>> Openembedded-devel@lists.openembedded.org
>> http://lists.openembedded.org/mailman/listinfo/openembedded-devel
>>      
>    

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

[Joe MacDonald]

[-- Attachment #1: Type: text/plain, Size: 10348 bytes --]

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@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
> >>_______________________________________________
> >>Openembedded-devel mailing list
> >>Openembedded-devel@lists.openembedded.org
> >>http://lists.openembedded.org/mailman/listinfo/openembedded-devel
> 

-- 
-Joe MacDonald.
:wq

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 205 bytes --]

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

[Xufeng Zhang]

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@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
>>>>>>              
>>>> _______________________________________________
>>>> Openembedded-devel mailing list
>>>> Openembedded-devel@lists.openembedded.org
>>>> http://lists.openembedded.org/mailman/listinfo/openembedded-devel
>>>>          
>>      
>    

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

[Joe MacDonald]

[-- Attachment #1: Type: text/plain, Size: 11308 bytes --]

[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@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
> >>>>_______________________________________________
> >>>>Openembedded-devel mailing list
> >>>>Openembedded-devel@lists.openembedded.org
> >>>>http://lists.openembedded.org/mailman/listinfo/openembedded-devel
> 

-- 
-Joe MacDonald.
:wq

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 205 bytes --]

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

[Joe MacDonald]

[-- Attachment #1: Type: text/plain, Size: 12073 bytes --]

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@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
> > >>>>_______________________________________________
> > >>>>Openembedded-devel mailing list
> > >>>>Openembedded-devel@lists.openembedded.org
> > >>>>http://lists.openembedded.org/mailman/listinfo/openembedded-devel
> > 
> 
-- 
-Joe MacDonald.
:wq

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 205 bytes --]