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

ospfd: fix state location mixup #16361

Merged
merged 2 commits into from
Jul 16, 2024

Conversation

eqvinox
Copy link
Contributor

@eqvinox eqvinox commented Jul 10, 2024

In the "2x2 matrix" of these, I accidentally edited "row-wise" when I should've edited "column-wise"... sigh

Reported-by: @rbfnet
Fixes: #16349
Fixes: 110945b ("ospfd: fix GR state location")

In the "2x2 matrix" of these, I accidentally edited "row-wise" when I
should've edited "column-wise"...  *sigh*

Reported-by: github.com/rbfnet
Fixes: FRRouting#16349
Fixes: 110945b ("ospfd: fix GR state location")
Signed-off-by: David Lamparter <[email protected]>

/* this one includes the path... because the instance number was in the path
* before :( ... which totally didn't have a mkdir anywhere.
*
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't see problems with the C logic but I'd rather see a comment explaining the usage and rationale behind the various paths. Your comment as written seems only useful in the context of what you are changing as opposed to what paths are created and what they are to be used for.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done (but I've put it in lib/libfrr.h because the variables are declared there & used beyond ospfd)

It's not immediately obvious what exactly the `frr_*dir` variables
exported from lib/libfrr.c are for.  Add a little text each to clarify.

Signed-off-by: David Lamparter <[email protected]>
@frrbot frrbot bot added the libfrr label Jul 11, 2024
Copy link
Collaborator

@aceelindem aceelindem 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 see one question.

OSPFD_COMPAT_STATE_NAME);
/* no COMPAT2 here since it was reading that was broken,
Copy link
Collaborator

Choose a reason for hiding this comment

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

What does "it was reading that was broken" mean?

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

@riw777 riw777 merged commit cf7c451 into FRRouting:master Jul 16, 2024
13 checks passed
@eqvinox eqvinox deleted the ospfd-state-loc-snafu branch July 26, 2024 00:42
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.

Version 10 ospfd state directory location inconsistency between no-instance-id and instance-id configurations
3 participants