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

ospfd: fix state location mixup #16361

Merged
merged 2 commits into from
Jul 16, 2024
Merged

ospfd: fix state location mixup #16361

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
54b7202
ospfd: fix state location mixup
eqvinox Jul 10, 2024
6ade526
lib: add some quick explainers for path vars
eqvinox Jul 11, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
30 changes: 22 additions & 8 deletions ospfd/ospf_main.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,14 +48,20 @@
#include "ospfd/ospf_apiserver.h"

#define OSPFD_STATE_NAME "%s/ospfd.json", frr_libstatedir
#define OSPFD_INST_STATE_NAME(i) "%s/ospfd-%d.json", frr_runstatedir, i
#define OSPFD_INST_STATE_NAME(i) "%s/ospfd-%d.json", frr_libstatedir, i

/* 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)

* ... and libstatedir & runstatedir got switched around while changing this;
* for non-instance it read the wrong path, for instance it wrote the wrong
* path. (There is no COMPAT2 for non-instance because it was writing to the
* right place, i.e. no extra path to check exists from reading a wrong path.)
*/
#define OSPFD_COMPAT_STATE_NAME "%s/ospfd-gr.json", frr_libstatedir
#define OSPFD_COMPAT_INST_STATE_NAME(i) \
#define OSPFD_COMPAT_STATE_NAME "%s/ospfd-gr.json", frr_runstatedir
#define OSPFD_COMPAT1_INST_STATE_NAME(i) \
"%s-%d/ospfd-gr.json", frr_runstatedir, i
#define OSPFD_COMPAT2_INST_STATE_NAME(i) "%s/ospfd-%d.json", frr_runstatedir, i

/* ospfd privileges */
zebra_capabilities_t _caps_p[] = {ZCAP_NET_RAW, ZCAP_BIND, ZCAP_NET_ADMIN,
Expand Down Expand Up @@ -139,10 +145,12 @@ static const struct frr_yang_module_info *const ospfd_yang_modules[] = {

/* actual paths filled in main() */
static char state_path[512];
static char state_compat_path[512];
static char state_compat1_path[512];
static char state_compat2_path[512];
static char *state_paths[] = {
state_path,
state_compat_path,
state_compat1_path,
state_compat2_path, /* NULLed out if not needed */
NULL,
};

Expand Down Expand Up @@ -242,12 +250,18 @@ int main(int argc, char **argv)
if (ospf_instance) {
snprintf(state_path, sizeof(state_path),
OSPFD_INST_STATE_NAME(ospf_instance));
snprintf(state_compat_path, sizeof(state_compat_path),
OSPFD_COMPAT_INST_STATE_NAME(ospf_instance));
snprintf(state_compat1_path, sizeof(state_compat1_path),
OSPFD_COMPAT1_INST_STATE_NAME(ospf_instance));
snprintf(state_compat2_path, sizeof(state_compat2_path),
OSPFD_COMPAT2_INST_STATE_NAME(ospf_instance));
} else {
snprintf(state_path, sizeof(state_path), OSPFD_STATE_NAME);
snprintf(state_compat_path, sizeof(state_compat_path),
snprintf(state_compat1_path, sizeof(state_compat1_path),
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?

* there is no additional path that would've been written
*/
state_paths[2] = NULL;
}

/* OSPF master init. */
Expand Down
Loading