From 54b72028c6059c8c69ec77335a5e828884fffbfe Mon Sep 17 00:00:00 2001 From: David Lamparter Date: Wed, 10 Jul 2024 09:49:51 +0200 Subject: [PATCH 1/2] ospfd: fix state location mixup 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: #16349 Fixes: 110945ba0d2 ("ospfd: fix GR state location") Signed-off-by: David Lamparter --- ospfd/ospf_main.c | 30 ++++++++++++++++++++++-------- 1 file changed, 22 insertions(+), 8 deletions(-) diff --git a/ospfd/ospf_main.c b/ospfd/ospf_main.c index fdb4e5c5875f..5c11027506dd 100644 --- a/ospfd/ospf_main.c +++ b/ospfd/ospf_main.c @@ -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. + * + * ... 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, @@ -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, }; @@ -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, + * there is no additional path that would've been written + */ + state_paths[2] = NULL; } /* OSPF master init. */ From 6ade526f7b14a725d61c98700d403dea050ccd85 Mon Sep 17 00:00:00 2001 From: David Lamparter Date: Thu, 11 Jul 2024 05:38:36 +0200 Subject: [PATCH 2/2] lib: add some quick explainers for path vars 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 --- lib/libfrr.h | 29 +++++++++++++++++++++++++++++ 1 file changed, 29 insertions(+) diff --git a/lib/libfrr.h b/lib/libfrr.h index db9cfbcb1f2b..8018672c1ae7 100644 --- a/lib/libfrr.h +++ b/lib/libfrr.h @@ -222,10 +222,39 @@ extern void frr_fini(void); extern char config_default[512]; extern char frr_zclientpath[512]; + +/* refer to lib/config_paths.h (generated during ./configure) for build config + * values of the following: + */ + +/* sysconfdir is generally /etc/frr/, some BSDs may use /usr/local/etc/frr/. + * Will NOT include "pathspace" (namespace) suffix from -N. (libfrr.c handles + * pathspace'ing config files.) Has a slash at the end for "historical" + * reasons. + */ extern const char frr_sysconfdir[]; + +/* runstatedir is *ephemeral* across reboots. It may either be a ramdisk, + * or be wiped during boot. Use only for pid files, sockets, and the like, + * not state. Commonly /run/frr or /var/run/frr. + * Will include "pathspace" (namespace) suffix from -N. + */ extern char frr_runstatedir[256]; + +/* libstatedir is *persistent*. It's the place to put state like sequence + * numbers or databases. Commonly /var/lib/frr. + * Will include "pathspace" (namespace) suffix from -N. + */ extern char frr_libstatedir[256]; + +/* moduledir is something along the lines of /usr/lib/frr/modules or + * /usr/lib/x86_64-linux-gnu/frr/modules. It is not guaranteed to be a + * subdirectory of the directory that the daemon binaries reside in. (e.g. + * the "x86_64-linux-gnu" component will be absent from daemon paths.) + */ extern const char frr_moduledir[]; + +/* scriptdir is for Lua scripts, generally ${frr_sysconfdir}/scripts */ extern const char frr_scriptdir[]; extern char frr_protoname[];