| Message ID | 09f171c8308cfaec4620c038b651cd6886f74bd8.1523601814.git.lucien.xin@gmail.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
Fri, Apr 13, 2018 at 08:43:34AM CEST, lucien.xin@gmail.com wrote: >When a dev is being unregistered and the corresp team port is being >removed, the ifinfo should be delayed to destroy when doing dellink >next time, because later on the dbg log still needs it to print the >ifname saved in this ifinfo: > ... > <port_list> > -1450: veth1: down 0Mbit HD <-- > </port_list> > ... > >The smilar process is also used on the team_port's destruction. > >However, after Commit 046fb6ba0aec ("libteam: resynchronize ifinfo >after lost RTNLGRP_LINK notifications"), it would destroy all those >ifinfo with REMVOED flag, including the one of current unregistered >dev enslaved into team. It would cause a crash later when trying to >print the dbg log. > >A simple way to reproduce: > > # ip link add veth1 type veth peer name veth1_peer > # sleep 5 && ip link del veth1 & > # teamd -g -c '{"device":"team0","runner":{"name":"activebackup"}, > "ports":{"veth1":{}}}' > >This patch is to avoid it by simply checking !ifinfo->port before >calling ifinfo_destroy in ifinfo_destroy_removed(), as well as in >ifinfo_clear_changed(), so that the ifinfo of current unregistered >dev enslaved into team will be delayed to destroy next time. What is "next time"? Please take case of the typos like: REMVOED, smilar Also, please be imparative in the patch description like you would be telling the codebase what to do.
On Thu, Apr 19, 2018 at 10:46 PM, Jiri Pirko <jiri@resnulli.us> wrote: > Fri, Apr 13, 2018 at 08:43:34AM CEST, lucien.xin@gmail.com wrote: >>When a dev is being unregistered and the corresp team port is being >>removed, the ifinfo should be delayed to destroy when doing dellink >>next time, because later on the dbg log still needs it to print the >>ifname saved in this ifinfo: >> ... >> <port_list> >> -1450: veth1: down 0Mbit HD <-- >> </port_list> >> ... >> >>The smilar process is also used on the team_port's destruction. >> >>However, after Commit 046fb6ba0aec ("libteam: resynchronize ifinfo >>after lost RTNLGRP_LINK notifications"), it would destroy all those >>ifinfo with REMVOED flag, including the one of current unregistered >>dev enslaved into team. It would cause a crash later when trying to >>print the dbg log. >> >>A simple way to reproduce: >> >> # ip link add veth1 type veth peer name veth1_peer >> # sleep 5 && ip link del veth1 & >> # teamd -g -c '{"device":"team0","runner":{"name":"activebackup"}, >> "ports":{"veth1":{}}}' >> >>This patch is to avoid it by simply checking !ifinfo->port before >>calling ifinfo_destroy in ifinfo_destroy_removed(), as well as in >>ifinfo_clear_changed(), so that the ifinfo of current unregistered >>dev enslaved into team will be delayed to destroy next time. > > What is "next time"? next time when ifinfo_destroy_removed is called for another ifinfo's change. > > > Please take case of the typos like: > REMVOED, smilar Sorry. > > Also, please be imparative in the patch description like you would be > telling the codebase what to do. Copy, 'be imparative', will improve the changelog and post v2. Thanks.
diff --git a/libteam/ifinfo.c b/libteam/ifinfo.c index 5c32a9c..d47c2bf 100644 --- a/libteam/ifinfo.c +++ b/libteam/ifinfo.c @@ -211,7 +211,8 @@ void ifinfo_clear_changed(struct team_handle *th) struct team_ifinfo *ifinfo; list_for_each_node_entry(ifinfo, &th->ifinfo_list, list) - clear_changed(ifinfo); + if (!ifinfo->port) + clear_changed(ifinfo); } static struct team_ifinfo *ifinfo_find_create(struct team_handle *th, @@ -245,7 +246,7 @@ void ifinfo_destroy_removed(struct team_handle *th) struct team_ifinfo *ifinfo, *tmp; list_for_each_node_entry_safe(ifinfo, tmp, &th->ifinfo_list, list) { - if (is_changed(ifinfo, CHANGED_REMOVED)) + if (is_changed(ifinfo, CHANGED_REMOVED) && !ifinfo->port) ifinfo_destroy(ifinfo); } }
When a dev is being unregistered and the corresp team port is being removed, the ifinfo should be delayed to destroy when doing dellink next time, because later on the dbg log still needs it to print the ifname saved in this ifinfo: ... <port_list> -1450: veth1: down 0Mbit HD <-- </port_list> ... The smilar process is also used on the team_port's destruction. However, after Commit 046fb6ba0aec ("libteam: resynchronize ifinfo after lost RTNLGRP_LINK notifications"), it would destroy all those ifinfo with REMVOED flag, including the one of current unregistered dev enslaved into team. It would cause a crash later when trying to print the dbg log. A simple way to reproduce: # ip link add veth1 type veth peer name veth1_peer # sleep 5 && ip link del veth1 & # teamd -g -c '{"device":"team0","runner":{"name":"activebackup"}, "ports":{"veth1":{}}}' This patch is to avoid it by simply checking !ifinfo->port before calling ifinfo_destroy in ifinfo_destroy_removed(), as well as in ifinfo_clear_changed(), so that the ifinfo of current unregistered dev enslaved into team will be delayed to destroy next time. Fixes: 046fb6ba0aec ("libteam: resynchronize ifinfo after lost RTNLGRP_LINK notifications") Signed-off-by: Xin Long <lucien.xin@gmail.com> --- libteam/ifinfo.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) -- 2.1.0