Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ripd: fix ip rip send/receive version command #18217

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Shbinging
Copy link
Contributor

@Shbinging Shbinging commented Feb 21, 2025

Now the vtysh will reject illegal command ip rip send/receive version 2 1, instead accept command ip rip send/receive version 1 2 as shown in doc.
Add topotest rip_send_recv_version test.
Signed-off-by: Shbinging [email protected]

@frrbot frrbot bot added the rip label Feb 21, 2025
@frrbot frrbot bot added the bugfix label Feb 21, 2025
@Shbinging Shbinging changed the title ripd:fix ip rip send/receive version command ripd: fix ip rip send/receive version command Feb 21, 2025
Copy link
Member

@ton31337 ton31337 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you add a topotest?

@Shbinging
Copy link
Contributor Author

Can you add a topotest?

OK,I added the test rip_send_recv_version

Copy link
Member

@ton31337 ton31337 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why don't we have a command of just saying ip rip receive version all or (both)?

@Shbinging
Copy link
Contributor Author

Shbinging commented Feb 21, 2025

Why don't we have a command of just saying ip rip receive version all or (both)?

Yes, we have command ip rip receive version 1 2 which means both , but not have ip rip receive version 2 1

Besides, we have command router rip version 1|2 but not have version 1 2, may be I can give a PR to add version 1 2 command.

@Shbinging Shbinging requested a review from ton31337 February 25, 2025 07:49
Copy link
Member

@riw777 riw777 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks good

IP_STR
"Routing Information Protocol\n"
"Advertisement reception\n"
"Version control\n"
"RIP version 1\n"
"RIP version 2\n"
"RIP version 1&2\n"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Align please.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From my editor this line 747 and 754 are both aligned

# SPDX-License-Identifier: ISC

# Copyright (c) 2023 by
# Donatas Abraitis <[email protected]>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not me.

#!/usr/bin/env python
# SPDX-License-Identifier: ISC

# Copyright (c) 2023 by
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

2025

#

"""
Test if RIP `passive-interface default` and `no passive-interface IFNAME`
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wrong description.

tgen.stop_topology()


import re
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Move imports to the top.



import re
def getConfOfInterface(i_name, text):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please use snake_case pattern for function names.

Now the vtysh will reject illegal command `ip rip send/receive version 2 1`, instead accept command `ip rip send/receive version 1 2` as shown in doc.
Add topotest `rip_send_recv_version` test.
Signed-off-by: Shbinging <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants