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 <paulz@labn.net>

diff --git a/ldpd/ldpd.c b/ldpd/ldpd.c
index 000d1a33019202b656bc667ba789e6322798fc1d..e24eba7cd438f05a44d3178105ec0948f6fdc16a 100644 (file)
--- 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 d03437328b83cec356ab58da5994770b7b86668d..9b05bb4fbf96828e733fb04809e39dbe03f1d0cd 100644 (file)
--- 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 1d51a6396d97efebc105db389aafdb89023f095a..4037bfbeb0202edc6d1661dadc5d03aee1c2b13a 100644 (file)
--- 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 6275877cb37423e6b9919202f0cafaded6cba5bf..ae1ca2f7579fb30eceb56ea3a208232ad06fd278 100644 (file)
--- 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 491796802a2375acb262efe968beccd7daf024b2..0aa1bbb7e114bfe6728019eb4d5ca21d5c5a4b23 100644 (file)
--- 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();