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.

Sign up for GitHub

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

Jump to bottom

vrrpd: use CS2MS instead of constant 10 everywhere #5181

Merged
merged 1 commit into from
Oct 17, 2019
Merged

vrrpd: use CS2MS instead of constant 10 everywhere #5181

merged 1 commit into from
Oct 17, 2019

Conversation

ghasemnaddaf
Copy link
Contributor

No description provided.

sworleys
sworleys approved these changes Oct 17, 2019
View reviewed changes
Copy link
Member

@sworleys sworleys left a comment

Choose a reason for hiding this comment

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

Other than putting it in the debugs as well, LGTM.

vrrpd/vrrp_vty.c Outdated
@@ -327,7 +327,7 @@ DEFPY(vrrp_default,
"Force VRRP router into administrative shutdown\n")
{
if (adv) {
if (advint % 10 != 0) {
if (advint % CS2MS != 0) {
vty_out(vty, "%% Value must be a multiple of 10\n");
Copy link
Member

Choose a reason for hiding this comment

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

Probably put it here in the string too.

vrrpd/vrrp_vty.c Outdated

if (newadvint % 10 != 0) {
if (newadvint % CS2MS != 0) {
vty_out(vty, "%% Value must be a multiple of 10\n");
Copy link
Member

Choose a reason for hiding this comment

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

same, in the string

@LabN-CI
Copy link
Collaborator

LabN-CI commented Oct 17, 2019

💚 Basic BGPD CI results: SUCCESS, 0 tests failed

Results table
_ _
Result SUCCESS git merge/5181 52cd6fc
Date 10/17/2019
Start 12:51:57
Finish 13:13:32
Run-Time 21:35
Total 1815
Pass 1815
Fail 0
Valgrind-Errors 0
Valgrind-Loss 0
Details vncregress-2019-10-17-12:51:57.txt
Log autoscript-2019-10-17-12:52:49.log.bz2
Memory 434 433 360

For details, please contact louberger

@LabN-CI
Copy link
Collaborator

LabN-CI commented Oct 17, 2019

💚 Basic BGPD CI results: SUCCESS, 0 tests failed

Results table
_ _
Result SUCCESS git merge/5181 44027d8
Date 10/17/2019
Start 13:36:59
Finish 13:58:44
Run-Time 21:45
Total 1815
Pass 1815
Fail 0
Valgrind-Errors 0
Valgrind-Loss 0
Details vncregress-2019-10-17-13:36:59.txt
Log autoscript-2019-10-17-13:37:53.log.bz2
Memory 432 435 359

For details, please contact louberger

@qlyoung
Copy link
Member

qlyoung commented Oct 17, 2019

lgtm, @ghasemnaddaf can you rebase and make it one commit though, thanks

@NetDEF-CI
Copy link
Collaborator

Continuous Integration Result: SUCCESSFUL

Congratulations, this patch passed basic tests

Tested-by: NetDEF / OpenSourceRouting.org CI System

CI System Testrun URL: https://ci1.netdef.org/browse/FRR-FRRPULLREQ-9290/

This is a comment from an automated CI system.
For questions and feedback in regards to this CI system, please feel free to email
Martin Winter - mwinter (at) opensourcerouting.org.

@NetDEF-CI
Copy link
Collaborator

Continuous Integration Result: SUCCESSFUL

Congratulations, this patch passed basic tests

Tested-by: NetDEF / OpenSourceRouting.org CI System

CI System Testrun URL: https://ci1.netdef.org/browse/FRR-FRRPULLREQ-9289/

This is a comment from an automated CI system.
For questions and feedback in regards to this CI system, please feel free to email
Martin Winter - mwinter (at) opensourcerouting.org.

CLANG Static Analyzer Summary

  • Github Pull Request 5181, comparing to Git base SHA 5b724d4

No Changes in Static Analysis warnings compared to base

1 Static Analyzer issues remaining.

See details at
https://ci1.netdef.org/browse/FRR-FRRPULLREQ-9289/artifact/shared/static_analysis/index.html

@gshirazi
Copy link

sure I can do that. That would be equivalent to squashing the PR instead of merging it.

@qlyoung
Copy link
Member

qlyoung commented Oct 17, 2019

Yes but we don't squash on github because it changes the commit sha and we prefer you to sign off on your own commit shas

@ghasemnaddaf
vrrpd: use CS2MS instead of constant 10 everywhere
de7fe61 
Signed-off-by: Ghasem Naddaf <ghasem.naddaf@gmail.com>

vrrpd: use CS2MS instead of constant 10 everywhere

Signed-off-by: Ghasem Naddaf <ghasem.naddaf@gmail.com>
@qlyoung qlyoung merged commit 2e6c9a1 into FRRouting:master Oct 17, 2019
@gshirazi
Copy link

good point on sign-off. thx. BTW can I be added to the dev slack channel please ? Thanks

@qlyoung
Copy link
Member

qlyoung commented Oct 17, 2019

https://frrouting.org/#participate slack invite link is there

@NetDEF-CI
Copy link
Collaborator

Continuous Integration Result: SUCCESSFUL

Congratulations, this patch passed basic tests

Tested-by: NetDEF / OpenSourceRouting.org CI System

CI System Testrun URL: https://ci1.netdef.org/browse/FRR-FRRPULLREQ-9298/

This is a comment from an automated CI system.
For questions and feedback in regards to this CI system, please feel free to email
Martin Winter - mwinter (at) opensourcerouting.org.

CLANG Static Analyzer Summary

  • Github Pull Request 5181, comparing to Git base SHA 0997ca9

Fixed warnings:

  • Dead store: Dead assignment in bgpd/bgp_evpn.c, function install_evpn_route_entry, line 2612
  • Logic error: Dereference of null pointer in nhrpd/nhrpd.h, function notifier_call, line 79

New warnings:

Static Analysis warning summary compared to base:

  • Fixed warnings: 2
  • New warnings: 1

1 Static Analyzer issues remaining.

See details at
https://ci1.netdef.org/browse/FRR-FRRPULLREQ-9298/artifact/shared/static_analysis/index.html

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sworleys sworleys sworleys approved these changes

Assignees
No one assigned
Labels
vrrp
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@ghasemnaddaf @LabN-CI @qlyoung @NetDEF-CI @gshirazi @sworleys @polychaeta