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

[Joe MacDonald <joe@deserted.net>]

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