ospf6d: rework ospf6 neighbor state display #14912

fdumontet6WIND · 2023-11-29T21:02:12Z

A separate function ospf6_neighbor_state_message() is created
to return a string that stands for the neighbor kind: DR, BDR,
DROther, or PointToPoint.

Add the following attributes in each neighbor of the vty command:
'show ipv6 ospf6 neighbor detail json'

nbrPriority : integer
Role: DR, BDR, DROther, or PointToPoint
nbrState : / where:
o Role is the above attribute
o state is "None", "Down", "Attempt", "Init", "Twoway",
"ExStart", "ExChange", "Loading", "Full"
routerDeadIntervalTimerDueMsec: integer value otherwise "inactive"

Enter in a deprecation workflow for neighborState value, which is
replaced by nbrState.
Add the following attributes in each neighbor of the vty command:
'show ipv6 ospf6 neighbor json':

nbrPriority : integer
Role: DR, BDR, DROther, or PointToPoint
nbrState : / where:
o Role is the above attribute
o state is "None", "Down", "Attempt", "Init", "Twoway",
"ExStart", "ExChange", "Loading", "Full"
routerDeadIntervalTimerDueMsec: integer value otherwise "inactive"

Enter in a deprecation workflow for state and priority values, which
are replaced by nbrState.

and update related tests

donaldsharp · 2023-11-29T23:14:47Z

ospf6d/ospf6_neighbor.c

@@ -892,6 +910,7 @@ static void ospf6_neighbor_show(struct vty *vty, struct ospf6_neighbor *on,
 		else
 			snprintf(nstate, sizeof(nstate), "DROther");
 	}
+	ospf6_neighbor_state_message(on, nstate, sizeof(nstate));


I'm not sure I really understand this change at all. This just completely blows away the nstate changes starting on line 900 and go to line 912. what's the goal here at all?

align the output state between ospfv2 and ospfv3.
we want to achieve it step by step.

taken into account

ton31337 · 2023-11-30T07:26:56Z

ospf6d/ospf6_neighbor.c

+static void ospf6_neighbor_nstate_message(struct ospf6_neighbor *on,
+					  char *nstate, size_t nstate_len)
+{
+	char state[16];


nit: char state[16] = {};. This way we don't need memset().

ton31337 · 2023-11-30T07:27:46Z

ospf6d/ospf6_neighbor.c

 		json_object_int_add(json_route, "priority", on->priority);
 		json_object_string_add(json_route, "deadTime", deadtime);
 		json_object_string_add(json_route, "state",
 				       ospf6_neighbor_state_str[on->state]);
+		json_object_int_add(json_route, "nbrPriority", on->priority);
+		json_object_string_add(json_route, "nbrState", nstate);
+		json_object_string_add(json_route, "Role",


lowercased please role

ton31337 · 2023-11-30T07:28:52Z

ospf6d/ospf6_neighbor.c

 		json_object_string_add(json_neighbor, "neighborState",
 				       ospf6_neighbor_state_str[on->state]);
+		json_object_int_add(json_neighbor, "nbrPriority", on->priority);
+		json_object_string_add(json_neighbor, "nbrState", nstate);
+		json_object_string_add(json_neighbor, "Role",


ton31337 · 2023-11-30T07:29:05Z

tests/topotests/ospf6_topo1/test_ospf6_topo1.py

@@ -174,7 +174,7 @@ def expect_neighbor_full(router, neighbor):
            topotest.router_json_cmp,
            tgen.gears[router],
            "show ipv6 ospf6 neighbor json",
-            {"neighbors": [{"neighborId": neighbor, "state": "Full"}]},
+            {"neighbors": [{"neighborId": neighbor, "Role": "Full"}]},


ton31337 · 2023-11-30T07:29:12Z

tests/topotests/ospf6_topo2/test_ospf6_topo2.py

@@ -157,7 +157,7 @@ def expect_neighbor_full(router, neighbor):
            topotest.router_json_cmp,
            tgen.gears[router],
            "show ipv6 ospf6 neighbor json",
-            {"neighbors": [{"neighborId": neighbor, "state": "Full"}]},
+            {"neighbors": [{"neighborId": neighbor, "Role": "Full"}]},


ton31337 · 2023-11-30T07:29:17Z

tests/topotests/ospf_topo1/test_ospf_topo1.py

@@ -154,7 +154,7 @@ def expect_ospfv3_neighbor_full(router, neighbor):
            topotest.router_json_cmp,
            tgen.gears[router],
            "show ipv6 ospf6 neighbor json",
-            {"neighbors": [{"neighborId": neighbor, "state": "Full"}]},
+            {"neighbors": [{"neighborId": neighbor, "Role": "Full"}]},


ton31337 · 2023-11-30T07:29:37Z

ospf6d/ospf6_neighbor.c

@@ -846,6 +846,39 @@ DEFPY(ipv6_ospf6_p2xp_neigh_poll_interval,

 	p2xp_unicast_hello_sched(p2xp_cfg);
 	return CMD_SUCCESS;
+


Can you remove this empty line?

ton31337 · 2023-11-30T07:33:34Z

ospf6d/ospf6_neighbor.c

+}
+
+/* build state value */
+static void ospf6_neighbor_state_message(struct ospf6_neighbor *on,


Can't we use static strings for this goal at all? Like:

static const char *ospf6_neighbor_state_message(...) { if (on->ospf6_if->type == OSPF_IFTYPE_POINTOPOINT) return "PointToPoint"; ... }

taken into account

ton31337 · 2023-12-01T11:58:32Z

ospf6d/ospf6_neighbor.c


 	inet_ntop(AF_INET6, &on->linklocal_addr, linklocal_addr,
 		  sizeof(linklocal_addr));
 	inet_ntop(AF_INET, &on->drouter, drouter, sizeof(drouter));
 	inet_ntop(AF_INET, &on->bdrouter, bdrouter, sizeof(bdrouter));

+	ospf6_neighbor_nstate_message(on, nstate, sizeof(nstate));


I'm not sure why we need to return the values via arguments here?

function remove following next comment

ton31337 · 2023-12-01T12:00:08Z

ospf6d/ospf6_neighbor.c

+}
+
+/* build nbrState value */
+static void ospf6_neighbor_nstate_message(struct ospf6_neighbor *on,


Why can't we print this information directly using json_object_string_addf()?

taken into account

ton31337 · 2023-12-01T12:00:34Z

ospf6d/ospf6_neighbor.c

@@ -848,16 +848,40 @@ DEFPY(ipv6_ospf6_p2xp_neigh_poll_interval,
 	return CMD_SUCCESS;
 }

+/* build state value */


What does it mean build state? :)

fdumontet6WIND · 2023-12-06T11:04:22Z

ci:rerun

ton31337 · 2023-12-08T09:33:06Z

ospf6d/ospf6_neighbor.c

+	if (on->router_id == on->bdrouter)
+		return "BR";
+	return "DROther";
+


nit: Drop this empty line, please.

ospf6d/ospf6_neighbor.c

ton31337 · 2023-12-08T09:34:15Z

ospf6d/ospf6_neighbor.c

 		json_object_int_add(json_route, "priority", on->priority);
 		json_object_string_add(json_route, "deadTime", deadtime);
 		json_object_string_add(json_route, "state",
 				       ospf6_neighbor_state_str[on->state]);
+		json_object_int_add(json_route, "nbrPriority", on->priority);
+		json_object_string_add(json_route, "nbrState", nstate);


Same here: let's use json_object_string_addf().

ospf6d/ospf6_neighbor.c

ton31337

Still some nits. Another question is: can we make identical changes for ospfv2? Just looks ridiculous when that differs (despite they are separate protocols).

ton31337 · 2023-12-12T14:37:46Z

ospf6d/ospf6_neighbor.c

@@ -63,6 +63,7 @@ const char *const ospf6_neighbor_event_str[] = {
 };

 int ospf6_neighbor_cmp(void *va, void *vb)
+


Please drop this change.

ospf6d/ospf6_neighbor.c

fdumontet6WIND · 2023-12-14T20:37:25Z

ci:rerun

fdumontet6WIND · 2023-12-29T22:00:48Z

ci:rerun

fdumontet6WIND · 2024-01-04T17:22:33Z

ci:
rerun

ton31337 · 2024-01-11T11:23:00Z

ospf6d/ospf6_neighbor.c

@@ -922,7 +949,8 @@ static void ospf6_neighbor_show(struct vty *vty, struct ospf6_neighbor *on,
 	} else
 		vty_out(vty, "%-15s %3d %11s %8s/%-12s %11s %s[%s]\n",


Is this enough/aligned with the new output?

in fact only json part is modified

ton31337 · 2024-01-11T11:25:35Z

ospfd/ospf_vty.c

-	if (uj)
+		if (uj)
+			vty_json(vty, json);
+	} else if (uj)
 		json_object_free(json);


I think we should call and print an empty JSON in such a case, right? vty_json_empty()

i use vty_json since it manage "/" caracter

mwinter-osr · 2024-01-14T23:26:05Z

CI:rerun Retest for new ASAN Memory Leaks

fdumontet6WIND · 2024-01-15T09:37:20Z

ci:rerun

github-actions · 2024-03-08T07:23:45Z

This pull request has conflicts, please resolve those before we can evaluate the pull request.

A separate function ospf6_neighbor_state_message() is created to return a string that stands for the neighbor kind: DR, BDR, DROther, or PointToPoint. Signed-off-by: Philippe Guibert <[email protected]>

With a 16 byte nstate value, the "PointToPoint" string is missing one extra-character to handle the full string and the termination character. Fix this by enlarging the nstate buffer size to 17 instead of 16. Fixes: 508e53e ("Ospf6d merge from Zebra repository with added privs stuff and merged zclient changes.") Signed-off-by: Philippe Guibert <[email protected]>

Add the following attributes in each neighbor of the vty command: 'show ipv6 ospf6 neighbor detail json' - nbrPriority : integer - Role: DR, BDR, DROther, or PointToPoint - nbrState : <state>/<Role> where: o Role is the above attribute o state is "None", "Down", "Attempt", "Init", "Twoway", "ExStart", "ExChange", "Loading", "Full" - routerDeadIntervalTimerDueMsec: integer value otherwise "inactive" Enter in a deprecation workflow for neighborState value, which is replaced by nbrState. Signed-off-by: Philippe Guibert <[email protected]>

Add the following attributes in each neighbor of the vty command: 'show ipv6 ospf6 neighbor json': - nbrPriority : integer - Role: DR, BDR, DROther, or PointToPoint - nbrState : <state>/<Role> where: o Role is the above attribute o state is "None", "Down", "Attempt", "Init", "Twoway", "ExStart", "ExChange", "Loading", "Full" - routerDeadIntervalTimerDueMsec: integer value otherwise "inactive" Enter in a deprecation workflow for state and priority values, which are replaced by nbrState. Signed-off-by: Philippe Guibert <[email protected]>

To handle the future change of naming in the ospf6 state, align the ospf6 tests with the new 'Role' nickname instead of the old 'state' value. Signed-off-by: Philippe Guibert <[email protected]>

github-actions · 2024-09-12T01:55:09Z

This PR is stale because it has been open 180 days with no activity. Comment or remove the autoclose label in order to avoid having this PR closed.

frrbot bot added bugfix ospfv3 labels Nov 29, 2023

github-actions bot added master size/M labels Nov 29, 2023

fdumontet6WIND force-pushed the rework_display branch 2 times, most recently from 2886f64 to 3cec1ba Compare November 29, 2023 21:54

donaldsharp reviewed Nov 29, 2023

View reviewed changes

ton31337 reviewed Nov 30, 2023

View reviewed changes

fdumontet6WIND force-pushed the rework_display branch from 3cec1ba to 36f564c Compare December 1, 2023 11:08

github-actions bot added size/L and removed size/M labels Dec 1, 2023

fdumontet6WIND force-pushed the rework_display branch 2 times, most recently from fb54a87 to 6cc8244 Compare December 1, 2023 11:47

github-actions bot added size/M and removed size/L labels Dec 1, 2023

ton31337 reviewed Dec 1, 2023

View reviewed changes

fdumontet6WIND force-pushed the rework_display branch from 6cc8244 to b277924 Compare December 5, 2023 15:02

github-actions bot added size/L and removed size/M labels Dec 5, 2023

fdumontet6WIND force-pushed the rework_display branch from b277924 to e54d248 Compare December 5, 2023 15:09

fdumontet6WIND force-pushed the rework_display branch from e54d248 to 7e45a7b Compare December 6, 2023 11:05

github-actions bot added the rebase PR needs rebase label Dec 6, 2023

ton31337 requested changes Dec 8, 2023

View reviewed changes

fdumontet6WIND force-pushed the rework_display branch from 7e45a7b to 2746555 Compare December 11, 2023 15:51

github-actions bot added size/M and removed size/L labels Dec 11, 2023

fdumontet6WIND force-pushed the rework_display branch from 2746555 to fab79cb Compare December 11, 2023 16:01

ton31337 requested changes Dec 12, 2023

View reviewed changes

github-actions bot added size/L and removed size/M labels Dec 13, 2023

fdumontet6WIND force-pushed the rework_display branch 3 times, most recently from 97ab54c to 3f3986b Compare December 14, 2023 20:10

fdumontet6WIND force-pushed the rework_display branch from 3f3986b to bd6fea6 Compare December 14, 2023 21:58

fdumontet6WIND force-pushed the rework_display branch 3 times, most recently from 700cddd to f6056ab Compare December 29, 2023 14:19

fdumontet6WIND force-pushed the rework_display branch from f6056ab to 6b53a6f Compare January 3, 2024 16:09

ton31337 reviewed Jan 11, 2024

View reviewed changes

fdumontet6WIND force-pushed the rework_display branch from 6b53a6f to 92f6973 Compare January 12, 2024 17:27

fdumontet6WIND requested a review from ton31337 January 22, 2024 14:04

fdumontet6WIND force-pushed the rework_display branch from 92f6973 to 2028898 Compare January 30, 2024 15:34

github-actions bot added the conflicts label Mar 8, 2024

pguibert6WIND added 5 commits March 15, 2024 14:08

ospf6d: rework ospf6 neighbor state display

0dc4f6f

A separate function ospf6_neighbor_state_message() is created to return a string that stands for the neighbor kind: DR, BDR, DROther, or PointToPoint. Signed-off-by: Philippe Guibert <[email protected]>

topotests: use the Role field when testing ospf6

bee2291

To handle the future change of naming in the ospf6 state, align the ospf6 tests with the new 'Role' nickname instead of the old 'state' value. Signed-off-by: Philippe Guibert <[email protected]>

fdumontet6WIND force-pushed the rework_display branch from 2028898 to bee2291 Compare March 15, 2024 13:11

github-actions bot removed the conflicts label Mar 15, 2024

github-actions bot added the autoclose label Sep 12, 2024

		@@ -846,6 +846,39 @@ DEFPY(ipv6_ospf6_p2xp_neigh_poll_interval,

		p2xp_unicast_hello_sched(p2xp_cfg);
		return CMD_SUCCESS;

		@@ -63,6 +63,7 @@ const char *const ospf6_neighbor_event_str[] = {
		};

		int ospf6_neighbor_cmp(void va, void vb)

		@@ -922,7 +949,8 @@ static void ospf6_neighbor_show(struct vty vty, struct ospf6_neighbor on,
		} else
		vty_out(vty, "%-15s %3d %11s %8s/%-12s %11s %s[%s]\n",

ospf6d: rework ospf6 neighbor state display #14912

Are you sure you want to change the base?

ospf6d: rework ospf6 neighbor state display #14912

Conversation

fdumontet6WIND commented Nov 29, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fdumontet6WIND commented Dec 6, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ton31337 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fdumontet6WIND commented Dec 14, 2023

fdumontet6WIND commented Dec 29, 2023

fdumontet6WIND commented Jan 4, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mwinter-osr commented Jan 14, 2024

fdumontet6WIND commented Jan 15, 2024

github-actions bot commented Mar 8, 2024

github-actions bot commented Sep 12, 2024