From 52fad8f6563080c64b5609f8b357ed47b1dfb78c Mon Sep 17 00:00:00 2001 From: "G. Paul Ziemba" Date: Fri, 3 Sep 2021 09:49:05 -0700 Subject: [PATCH] lib/module.c and callers of frrmod_load(): fix error messages frrmod_load() attempts to dlopen() several possible paths (constructed from its basename argument) until one succeeds. Each dlopen() attempt may fail for a different reason, and the important one might not be the last one. Example: dlopen(a/foo): file not found dlopen(b/foo): symbol "bar" missing dlopen(c/foo): file not found Previous code reported only the most recent error. Now frrmod_load() describes each dlopen() failure. Signed-off-by: G. Paul Ziemba --- ldpd/ldpd.c | 3 +- lib/libfrr.c | 14 ++++--- lib/module.c | 92 ++++++++++++++++++++++++++++++++++------- lib/module.h | 4 +- tests/lib/test_grpc.cpp | 18 ++++---- 5 files changed, 99 insertions(+), 32 deletions(-) diff --git a/ldpd/ldpd.c b/ldpd/ldpd.c index 000d1a3301..e24eba7cd4 100644 --- a/ldpd/ldpd.c +++ b/ldpd/ldpd.c @@ -94,10 +94,9 @@ static void ldp_load_module(const char *name) { const char *dir; dir = ldpd_di.module_path ? ldpd_di.module_path : frr_moduledir; - char moderr[256]; struct frrmod_runtime *module; - module = frrmod_load(name, dir, moderr, sizeof(moderr)); + module = frrmod_load(name, dir, NULL,NULL); if (!module) { fprintf(stderr, "%s: failed to load %s", __func__, name); log_warnx("%s: failed to load %s", __func__, name); diff --git a/lib/libfrr.c b/lib/libfrr.c index d03437328b..9b05bb4fbf 100644 --- a/lib/libfrr.c +++ b/lib/libfrr.c @@ -674,13 +674,19 @@ static void frr_mkdir(const char *path, bool strip) strerror(errno)); } +static void _err_print(const void *cookie, const char *errstr) +{ + const char *prefix = (const char *)cookie; + + fprintf(stderr, "%s: %s\n", prefix, errstr); +} + static struct thread_master *master; struct thread_master *frr_init(void) { struct option_chain *oc; struct frrmod_runtime *module; struct zprivs_ids_t ids; - char moderr[256]; char p_instance[16] = "", p_pathspace[256] = ""; const char *dir; dir = di->module_path ? di->module_path : frr_moduledir; @@ -734,11 +740,9 @@ struct thread_master *frr_init(void) frrmod_init(di->module); while (modules) { modules = (oc = modules)->next; - module = frrmod_load(oc->arg, dir, moderr, sizeof(moderr)); - if (!module) { - fprintf(stderr, "%s\n", moderr); + module = frrmod_load(oc->arg, dir, _err_print, __func__); + if (!module) exit(1); - } XFREE(MTYPE_TMP, oc); } diff --git a/lib/module.c b/lib/module.c index 1d51a6396d..4037bfbeb0 100644 --- a/lib/module.c +++ b/lib/module.c @@ -26,9 +26,11 @@ #include "module.h" #include "memory.h" #include "lib/version.h" +#include "printfrr.h" DEFINE_MTYPE_STATIC(LIB, MODULE_LOADNAME, "Module loading name"); DEFINE_MTYPE_STATIC(LIB, MODULE_LOADARGS, "Module loading arguments"); +DEFINE_MTYPE_STATIC(LIB, MODULE_LOAD_ERR, "Module loading error"); static struct frrmod_info frrmod_default_info = { .name = "libfrr", @@ -67,14 +69,64 @@ void frrmod_init(struct frrmod_runtime *modinfo) execname = modinfo->info->name; } -struct frrmod_runtime *frrmod_load(const char *spec, const char *dir, char *err, - size_t err_len) +/* + * If caller wants error strings, it should define non-NULL pFerrlog + * which will be called with 0-terminated error messages. These + * messages will NOT contain newlines, and the (*pFerrlog)() function + * could be called multiple times for a single call to frrmod_load(). + * + * The (*pFerrlog)() function may copy these strings if needed, but + * should expect them to be freed by frrmod_load() before frrmod_load() + * returns. + * + * frrmod_load() is coded such that (*pFerrlog)() will be called only + * in the case where frrmod_load() returns an error. + */ +struct frrmod_runtime *frrmod_load(const char *spec, const char *dir, + void (*pFerrlog)(const void *, const char *), + const void *pErrlogCookie) { void *handle = NULL; char name[PATH_MAX], fullpath[PATH_MAX * 2], *args; struct frrmod_runtime *rtinfo, **rtinfop; const struct frrmod_info *info; +#define FRRMOD_LOAD_N_ERRSTR 10 + char *aErr[FRRMOD_LOAD_N_ERRSTR]; + unsigned int iErr = 0; + + memset(aErr, 0, sizeof(aErr)); + +#define ERR_RECORD(...) \ + do { \ + if (pFerrlog && (iErr < FRRMOD_LOAD_N_ERRSTR)) { \ + aErr[iErr++] = asprintfrr(MTYPE_MODULE_LOAD_ERR, \ + __VA_ARGS__); \ + } \ + } while (0) + +#define ERR_REPORT \ + do { \ + if (pFerrlog) { \ + unsigned int i; \ + \ + for (i = 0; i < iErr; ++i) { \ + (*pFerrlog)(pErrlogCookie, aErr[i]); \ + } \ + } \ + } while (0) + +#define ERR_FREE \ + do { \ + unsigned int i; \ + \ + for (i = 0; i < iErr; ++i) { \ + XFREE(MTYPE_MODULE_LOAD_ERR, aErr[i]); \ + aErr[i] = 0; \ + } \ + iErr = 0; \ + } while (0) + snprintf(name, sizeof(name), "%s", spec); args = strchr(name, ':'); if (args) @@ -85,32 +137,41 @@ struct frrmod_runtime *frrmod_load(const char *spec, const char *dir, char *err, snprintf(fullpath, sizeof(fullpath), "%s/%s_%s.so", dir, execname, name); handle = dlopen(fullpath, RTLD_NOW | RTLD_GLOBAL); + if (!handle) + ERR_RECORD("loader error: dlopen(%s): %s", + fullpath, dlerror()); } if (!handle) { snprintf(fullpath, sizeof(fullpath), "%s/%s.so", dir, name); handle = dlopen(fullpath, RTLD_NOW | RTLD_GLOBAL); + if (!handle) + ERR_RECORD("loader error: dlopen(%s): %s", + fullpath, dlerror()); } } if (!handle) { snprintf(fullpath, sizeof(fullpath), "%s", name); handle = dlopen(fullpath, RTLD_NOW | RTLD_GLOBAL); + if (!handle) + ERR_RECORD("loader error: dlopen(%s): %s", fullpath, + dlerror()); } if (!handle) { - if (err) - snprintf(err, err_len, - "loading module \"%s\" failed: %s", name, - dlerror()); + ERR_REPORT; + ERR_FREE; return NULL; } + /* previous dlopen() errors are no longer relevant */ + ERR_FREE; + rtinfop = dlsym(handle, "frr_module"); if (!rtinfop) { dlclose(handle); - if (err) - snprintf(err, err_len, - "\"%s\" is not an FRR module: %s", name, - dlerror()); + ERR_RECORD("\"%s\" is not an FRR module: %s", name, dlerror()); + ERR_REPORT; + ERR_FREE; return NULL; } rtinfo = *rtinfop; @@ -122,17 +183,13 @@ struct frrmod_runtime *frrmod_load(const char *spec, const char *dir, char *err, if (rtinfo->finished_loading) { dlclose(handle); - if (err) - snprintf(err, err_len, "module \"%s\" already loaded", - name); + ERR_RECORD("module \"%s\" already loaded", name); goto out_fail; } if (info->init && info->init()) { dlclose(handle); - if (err) - snprintf(err, err_len, - "module \"%s\" initialisation failed", name); + ERR_RECORD("module \"%s\" initialisation failed", name); goto out_fail; } @@ -140,11 +197,14 @@ struct frrmod_runtime *frrmod_load(const char *spec, const char *dir, char *err, *frrmod_last = rtinfo; frrmod_last = &rtinfo->next; + ERR_FREE; return rtinfo; out_fail: XFREE(MTYPE_MODULE_LOADARGS, rtinfo->load_args); XFREE(MTYPE_MODULE_LOADNAME, rtinfo->load_name); + ERR_REPORT; + ERR_FREE; return NULL; } diff --git a/lib/module.h b/lib/module.h index 6275877cb3..ae1ca2f757 100644 --- a/lib/module.h +++ b/lib/module.h @@ -91,7 +91,9 @@ extern struct frrmod_runtime *frrmod_list; extern void frrmod_init(struct frrmod_runtime *modinfo); extern struct frrmod_runtime *frrmod_load(const char *spec, const char *dir, - char *err, size_t err_len); + void (*pFerrlog)(const void *, + const char *), + const void *pErrlogCookie); #if 0 /* not implemented yet */ extern void frrmod_unload(struct frrmod_runtime *module); diff --git a/tests/lib/test_grpc.cpp b/tests/lib/test_grpc.cpp index 491796802a..0aa1bbb7e1 100644 --- a/tests/lib/test_grpc.cpp +++ b/tests/lib/test_grpc.cpp @@ -81,11 +81,16 @@ static const struct frr_yang_module_info *const staticd_yang_modules[] = { static int grpc_thread_stop(struct thread *thread); +static void _err_print(const void *cookie, const char *errstr) +{ + std::cout << "Failed to load grpc module:" << errstr << std::endl; +} + static void static_startup(void) { // struct frrmod_runtime module; // static struct option_chain *oc; - char moderr[256] = {}; + cmd_init(1); zlog_aux_init("NONE: ", LOG_DEBUG); @@ -94,17 +99,14 @@ static void static_startup(void) /* Load the server side module -- check libtool path first */ std::string modpath = std::string(binpath) + std::string("../../../lib/.libs"); - grpc_module = frrmod_load("grpc:50051", modpath.c_str(), moderr, sizeof(moderr)); + grpc_module = frrmod_load("grpc:50051", modpath.c_str(), 0, 0); if (!grpc_module) { modpath = std::string(binpath) + std::string("../../lib"); - grpc_module = frrmod_load("grpc:50051", modpath.c_str(), moderr, - sizeof(moderr)); + grpc_module = frrmod_load("grpc:50051", modpath.c_str(), + _err_print, 0); } - if (!grpc_module) { - std::cout << "Failed to load grpc module:" << moderr - << std::endl; + if (!grpc_module) exit(1); - } static_debug_init(); -- 2.39.5