Eigrp #11301 - Configuration failed error type: validation

[1337kerberos]

For more details, please look at: #11301 (comment)
Additional context: https://vyos.dev/T5737

[1337kerberos]

Not sure if I need to fix the CI warnings, it is not dangerous to ignore identifier names for function declarations in the header. Unless maintainers would like me to handle this.

[donaldsharp]

The rule is that you don't have to fix something you don't touch but if you add new code it must conform to our standard. The tool is telling you about the problem

[ton31337]

Documentation and the tests are missing yet.

[ton31337]

If this is just returning, we might then use a void() at all?

[1337kerberos]

It is a hook definition; the declaration is expected to return int. The same goes for interface deletion.

[1337kerberos]

So, the pattern is clear.

  rg> ^int.*eigrp_if_  < 7/7 (0)
  eigrp_interface.c | 51 | int eigrp_if_new_hook(struct interface *ifp)                                                               
  eigrp_interface.c | 91 | int eigrp_if_delete_hook(struct interface *ifp)                                                            
  eigrp_interface.c | 111 | static int eigrp_ifp_create(struct interface *ifp)                                                        
  eigrp_interface.c | 126 | static int eigrp_ifp_up(struct interface *ifp)                                                            
  eigrp_interface.c | 167 | static int eigrp_ifp_down(struct interface *ifp)                                                          
  eigrp_interface.c | 179 | static int eigrp_ifp_destroy(struct interface *ifp)                                                                                                                 

  # returns a real 0/1 value, but no one handles
  eigrp_interface.c | 324 | int eigrp_if_down(struct eigrp_interface *ei)   
  eigrp_interface.c | 231 | int eigrp_if_up(struct eigrp_interface *ei)                                                               

You have a point - many return 0, so we can change it to void if you wish

[donaldsharp]

From the hook documentation:

 * The return value is always "int" for now;  hook_call will sum up the
 * return values from each registered user.  Default is 0.
 *
 * There are no pre-defined semantics for the value, in most cases it is
 * ignored.  For success/failure indication, 0 should be success, and
 * handlers should make sure to only return 0 or 1 (not -1 or other values).```

Leave a  return of 0 on success

[riw777]

looks good ... waiting on @ton31337 's comments to be resolved

[1337kerberos]

Regarding missing documentation - it is already present.

eigrpd.rst:
----------------------------------------------------------------------------------------------
.. clicmd:: passive-interface (IFNAME|default)

   This command sets the specified interface to passive mode. On passive mode
   interface, all receiving packets are ignored and eigrpd does not send either
   multicast or unicast EIGRP packets except to EIGRP neighbors specified with
   `neighbor` command. The interface may be specified as `default` to make
   eigrpd default to passive on all interfaces.

   The default is to be passive on all interfaces.

I have fixed the CI warnings and will check if somebody from my team can assist with the test case.

[riw777]

not certain we need a topo test here?

[riw777]

@volodymyrhuti if we can get the invalid interface bits fixed, we can merge this, I think ...

[github-actions]

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

[github-actions]

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

[riw777]

Is anyone still working on this?

[riw777]

@frrbot autoclose in 1 month

[1337kerberos]

@riw777 sorry for not following up on this one. I believe it is still relevant and applicable
I ignored it as I was expecting extra help with testing from the team.
At the moment, I`m not more associated with VyOS, so the testing part is irrelevant now.
If you think it looks good, I will find a time next week to rebase and run it locally.

[frrbot]

This issue will no longer be automatically closed.

[github-actions]

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.

[1337kerberos]

So, I fixed the index issue and returned 0 if no interface was configured.
However, I had to prepend the buffer with an empty element to sync indexing.
It makes sense, but I'm not a big fan.
In the prior solution, -1 means no element, and 0 is the first idx of the vector

static struct eigrp *eigrp_new(uint16_t as, vrf_id_t vrf_id)
	/* init internal data structures */
	eigrp->eiflist = list_new();
	eigrp->passive_nondefault = vector_init(1);
	vector_set(eigrp->passive_nondefault,
		   XSTRDUP(MTYPE_EIGRP_IF_STRING, "IFINDEX_INTERNAL"));

I'm going to run and verify that everything is ok, as I haven't touched this for a long time ...
Regarding the [CI] Basic Tests failing -> doesn't look related to my changes

lib/northbound_oper.c:146:25: warning: Dereference of null pointer [core.NullDereference]
  146 |                 *darr_last(ys->xpath) = 0;
      |                 ~~~~~~~~~~~~~~~~~~~~~~^~~
1 warning generated.
eigrpd/eigrp_interface.c:53:26: warning: Value stored to 'ei' during its initialization is never read [deadcode.DeadStores]
   53 |         struct eigrp_interface *ei = ifp->info;
      |                                 ^~   ~~~~~~~~~
1 warning generated.
----------------------------------------------------
2025-02-23 22:40:15,352 ERROR: topo: 'compare_show_ipv6' failed after 81.50 seconds
2025-02-23 22:40:15,361 ERROR: topo: test failed at "test_ospf6_point_to_multipoint/test_ospfv3_routingTable": OSPFv3 did not converge on r1:
  --- Current output
  +++ Expected output
  @@ -10,3 +10,4 @@
   O>* fc00:b:b:b::4/128 [110/20] via fe80::XXXX:XXXX:XXXX:XXXX, r1-sw5, weight 1, XX:XX:XX
   O>* fc00:2222:2222:2222::/64 [110/20] via fe80::XXXX:XXXX:XXXX:XXXX, r1-sw5, weight 1, XX:XX:XX
   O>* fc00:3333:3333:3333::/64 [110/20] via fe80::XXXX:XXXX:XXXX:XXXX, r1-sw5, weight 1, XX:XX:XX
  +O>* fc00:4444:4444:4444::/64 [110/20] via fe80::XXXX:XXXX:XXXX:XXXX, r1-sw5, weight 1, XX:XX:XX
assert False

Would someone be able to give this another look? I would much appreciate it, thanks

[1337kerberos]

Looking at the new tickets #18239, this could be a problem since most of the logic was taken from the RIP demon.
I want to get clarification from the maintainers on whether that is a bug so I can fix it in both places.

[donaldsharp]

I believe the test in eigrpd_instance_passive_interface_create in the validation part is just... wrong. The whole point of the validation stage is to check to see if anything would prevent a interface from being passive( ie some other config that would prevent it ) not that the ei has been created for the interface or not.

Changing the validation part would of course necessitate changing the APPLY part as well, in that the ei should be created there if it does not exist.. This of course brings out another problem with the eigrp implementation as a whole confabulates the idea of a eigrp_interface with the associated addresses that are interesting to eigrp. There needs to be another level of abstraction here to handle that. That would entail reaching deep into the eigrp code to fix... Which is why I haven't done it yet.

The approach that should be taken is that the network statement should create a series of eigrp_connected structures ( similiar in nature how the rest of FRR has the ifp and the ifp->connected structures ). Then when that is done appropriately any configuration that mentions a interface would just create a ei. When a network statement is issued the ifp's in the system would be iterated over and a series of eigrp_interface_addresses would be created.

As it stands If I were working on this issue, I would first get the ei data structure split out from the idea of a prefix and create the list of eigrp_connected structures that hang off the ei ). Then the rest of eigrp could be fixed to use this. Then fixing the UI would be very easy to do.

[1337kerberos]

Sure, @donaldsharp. Now that things are clear, I will invest some time into investigating how complicated that would be.
I will improve this, assuming I have time and enough skill. Thanks