log: split log variables from root object

The original plan was to avoid any global variables in the library. Thus
the logging variables were tied to the root object which is available
via the fabric API.

The default NVME API doesn't have the root object thus we had to add a
global variable to set log level etc. But the API to set these variables
is done via the root object.

This approach enforces the caller side to create an default root object
to initialize the default logging level. This introduces unnecessary
complexity on the caller side and wastes resources.

Let's split the default logging setting from the root object.

Signed-off-by: Daniel Wagner <dwagner@suse.de>

diff --git a/src/libnvme.map b/src/libnvme.map
index 95d9c98c8ddfe305c9399d3f16b820905cbe7c41..b610a8ffeddc81ea2f6f63ac9da99088d47d5196 100644 (file)
--- a/src/libnvme.map
+++ b/src/libnvme.map
@@ -1,4 +1,9 @@
 # SPDX-License-Identifier: LGPL-2.1-or-later
+LIBNVME_1.10 {
+       global:
+               nvme_init_default_logging;
+};
+
 LIBNVME_1.9 {
        global:
                nvme_ctrl_get_cntlid;

diff --git a/src/nvme/json.c b/src/nvme/json.c
index a02bd2d80f002b5fc55717b9567dc93dadb43522..e73260372c357ca2e95e77612601700c7055a448 100644 (file)
--- a/src/nvme/json.c
+++ b/src/nvme/json.c
@@ -633,7 +633,7 @@ int json_dump_tree(nvme_root_t r)
        }
        json_object_object_add(json_root, "hosts", host_array);
 
-       ret = json_object_to_fd(fileno(r->fp), json_root, JSON_C_TO_STRING_PRETTY);
+       ret = json_object_to_fd(r->log.fd, json_root, JSON_C_TO_STRING_PRETTY);
        if (ret < 0) {
                nvme_msg(r, LOG_ERR, "Failed to write, %s\n",
                         json_util_get_last_err());

diff --git a/src/nvme/log.c b/src/nvme/log.c
index d3f84104225a2d0c0035833646fa4862f36660eb..eaf74e1d0cf9d860e848c9785a2238c985d74d37 100644 (file)
--- a/src/nvme/log.c
+++ b/src/nvme/log.c
@@ -26,13 +26,18 @@
 #define LOG_CLOCK CLOCK_MONOTONIC
 #endif
 
-static nvme_root_t root;
+static struct nvme_log def_log = {
+       .fd = STDERR_FILENO,
+       .level = DEFAULT_LOGLEVEL,
+       .pid = false,
+       .timestamp = false,
+};
 
 void __attribute__((format(printf, 4, 5)))
 __nvme_msg(nvme_root_t r, int level,
           const char *func, const char *format, ...)
 {
-       FILE *fp = stderr;
+       struct nvme_log *l;
        va_list ap;
        char pidbuf[16];
        char timebuf[32];
@@ -50,18 +55,15 @@ __nvme_msg(nvme_root_t r, int level,
        _cleanup_free_ char *message = NULL;
        int idx = 0;
 
-       if (!r)
-               r = root;
-
        if (r)
-               fp = r->fp;
+               l = &r->log;
+       else
+               l = &def_log;
 
-       if (r && level > r->log_level)
-               return;
-       if (!r && level > DEFAULT_LOGLEVEL)
+       if (level > l->level)
                return;
 
-       if (r && r->log_timestamp) {
+       if (l->timestamp) {
                struct timespec now;
 
                clock_gettime(LOG_CLOCK, &now);
@@ -71,7 +73,7 @@ __nvme_msg(nvme_root_t r, int level,
        } else
                *timebuf = '\0';
 
-       if (r && r->log_pid) {
+       if (l->pid) {
                snprintf(pidbuf, sizeof(pidbuf), "%ld", (long)getpid());
                idx |= 1 << 1;
        } else
@@ -89,42 +91,56 @@ __nvme_msg(nvme_root_t r, int level,
                message = NULL;
        va_end(ap);
 
-       fprintf(fp, "%s%s",
+       dprintf(l->fd, "%s%s",
                header ? header : "<error>",
                message ? message : "<error>");
 }
 
 void nvme_init_logging(nvme_root_t r, int lvl, bool log_pid, bool log_tstamp)
 {
-       r->log_level = lvl;
-       r->log_pid = log_pid;
-       r->log_timestamp = log_tstamp;
+       r->log.level = lvl;
+       r->log.pid = log_pid;
+       r->log.timestamp = log_tstamp;
 }
 
 int nvme_get_logging_level(nvme_root_t r, bool *log_pid, bool *log_tstamp)
 {
-       if (!r)
-               r = root;
-       if (!r)
-               return DEFAULT_LOGLEVEL;
+       struct nvme_log *l;
+
+       if (r)
+               l = &r->log;
+       else
+               l = &def_log;
+
        if (log_pid)
-               *log_pid = r->log_pid;
+               *log_pid = l->pid;
        if (log_tstamp)
-               *log_tstamp = r->log_timestamp;
-       return r->log_level;
+               *log_tstamp = l->timestamp;
+       return l->level;
+}
+
+void nvme_init_default_logging(FILE *fp, int level, bool log_pid, bool log_tstamp)
+{
+       def_log.fd = fileno(fp);
+       def_log.level = level;
+       def_log.pid = log_pid;
+       def_log.timestamp = log_tstamp;
 }
 
 void nvme_set_root(nvme_root_t r)
 {
-       root = r;
+       def_log.fd = r->log.fd;
+       def_log.level = r->log.level;
+       def_log.pid = r->log.pid;
+       def_log.timestamp = r->log.timestamp;
 }
 
 void nvme_set_debug(bool debug)
 {
-       root->log_level = debug ? LOG_DEBUG : DEFAULT_LOGLEVEL;
+       def_log.level = debug ? LOG_DEBUG : DEFAULT_LOGLEVEL;
 }
 
 bool nvme_get_debug(void)
 {
-       return root->log_level == LOG_DEBUG;
+       return def_log.level == LOG_DEBUG;
 }

diff --git a/src/nvme/log.h b/src/nvme/log.h
index cd243ea9f4a79117a7cd78d376c1cf1b6f9423dd..80c642aa4204d4e31b8cc0e4698de6511e6d73ac 100644 (file)
--- a/src/nvme/log.h
+++ b/src/nvme/log.h
@@ -35,6 +35,18 @@
  */
 void nvme_init_logging(nvme_root_t r, int lvl, bool log_pid, bool log_tstamp);
 
+/**
+ * nvme_init_default_logging() - Initialize default (fallback) logging
+ * @fp:                File descriptor for logging messages
+ * @lvl:       Logging level to set
+ * @log_pid:   Boolean to enable logging of the PID
+ * @log_tstamp:        Boolean to enable logging of the timestamp
+ *
+ * Sets the default logging settings for the library in case the root object
+ * is absent.
+ */
+void nvme_init_default_logging(FILE *fp, int lvl, bool log_pid, bool log_tstamp);
+
 /**
  * nvme_get_logging_level() - Get current logging level
  * @r:         nvme_root_t context
@@ -59,24 +71,27 @@ int nvme_get_logging_level(nvme_root_t r, bool *log_pid, bool *log_tstamp);
  * will be set as well. This means the global root object is always pointing to
  * the latest created root object. Note the first @nvme_free_tree call will reset
  * the global root object.
+ *
+ * This function is deprecated. Use nvme_init_default_logging or/and
+ * nvme_init_logging instead.
  */
-void nvme_set_root(nvme_root_t r);
+void nvme_set_root(nvme_root_t r) __attribute__((deprecated));
 
 /**
  * nvme_set_debug - Set NVMe command debugging output
  * @debug:     true to enable or false to disable
  *
- * Don't use it, it's debricated.
+ * This function is deprecated. Use nvme_init_default_logging instead.
  */
-void nvme_set_debug(bool debug);
+void nvme_set_debug(bool debug) __attribute__((deprecated));
 
 /**
  * nvme_get_debug - Get NVMe command debugging output
  *
- * Don't use it, it's debricated.
+ * This function is deprecated. Use nvme_get_logging_level instead.
  *
  * Return: false if disabled or true if enabled.
  */
-bool nvme_get_debug(void);
+bool nvme_get_debug(void) __attribute__((deprecated));
 
 #endif /* _LOG_H */

diff --git a/src/nvme/mi.c b/src/nvme/mi.c
index 3793ad46bebfe786c44be272c0e6878effe99350..2635de50928e04292289e4cc3039245228e23b49 100644 (file)
--- a/src/nvme/mi.c
+++ b/src/nvme/mi.c
@@ -11,6 +11,7 @@
 #include <stdlib.h>
 #include <stdio.h>
 #include <time.h>
+#include <unistd.h>
 
 #include <ccan/array_size/array_size.h>
 #include <ccan/endian/endian.h>
@@ -41,18 +42,32 @@ static bool nvme_mi_probe_enabled_default(void)
  */
 nvme_root_t nvme_mi_create_root(FILE *fp, int log_level)
 {
-       struct nvme_root *r = calloc(1, sizeof(*r));
+       struct nvme_root *r;
+       int fd;
 
+       r = calloc(1, sizeof(*r));
        if (!r) {
+               errno = ENOMEM;
                return NULL;
        }
-       r->log_level = log_level;
-       r->fp = stderr;
+
+       if (fp) {
+               fd = fileno(fp);
+               if (fd < 0) {
+                       free(r);
+                       return NULL;
+               }
+       } else
+               fd = STDERR_FILENO;
+
+       r->log.fd = fd;
+       r->log.level = log_level;
+
        r->mi_probe_enabled = nvme_mi_probe_enabled_default();
-       if (fp)
-               r->fp = fp;
+
        list_head_init(&r->hosts);
        list_head_init(&r->endpoints);
+
        return r;
 }
 

diff --git a/src/nvme/private.h b/src/nvme/private.h
index 18363502f9039b2ce44241a08366bfb0536e1f05..f2d7b5bebfc75e0dad8b6fcc667162907ef741db 100644 (file)
--- a/src/nvme/private.h
+++ b/src/nvme/private.h
@@ -160,15 +160,19 @@ struct nvme_fabric_options {
        bool trsvcid;
 };
 
+struct nvme_log {
+       int fd;
+       int level;
+       bool pid;
+       bool timestamp;
+};
+
 struct nvme_root {
        char *config_file;
        char *application;
        struct list_head hosts;
        struct list_head endpoints; /* MI endpoints */
-       FILE *fp;
-       int log_level;
-       bool log_pid;
-       bool log_timestamp;
+       struct nvme_log log;
        bool modified;
        bool mi_probe_enabled;
        struct nvme_fabric_options *options;

diff --git a/src/nvme/tree.c b/src/nvme/tree.c
index eb9486dec251334a78ec82edc045958f5aa1c8a8..d3ce49d83fd879da2f5427f672628947d912dbf5 100644 (file)
--- a/src/nvme/tree.c
+++ b/src/nvme/tree.c
@@ -187,19 +187,30 @@ int nvme_scan_topology(struct nvme_root *r, nvme_scan_filter_t f, void *f_args)
 
 nvme_root_t nvme_create_root(FILE *fp, int log_level)
 {
-       struct nvme_root *r = calloc(1, sizeof(*r));
+       struct nvme_root *r;
+       int fd;
 
+       r = calloc(1, sizeof(*r));
        if (!r) {
                errno = ENOMEM;
                return NULL;
        }
-       r->log_level = log_level;
-       r->fp = stderr;
-       if (fp)
-               r->fp = fp;
+
+       if (fp) {
+               fd = fileno(fp);
+               if (fd < 0) {
+                       free(r);
+                       return NULL;
+               }
+       } else
+               fd = STDERR_FILENO;
+
+       r->log.fd = fd;
+       r->log.level = log_level;
+
        list_head_init(&r->hosts);
        list_head_init(&r->endpoints);
-       nvme_set_root(r);
+
        return r;
 }
 
@@ -368,7 +379,6 @@ void nvme_free_tree(nvme_root_t r)
                free(r->config_file);
        if (r->application)
                free(r->application);
-       nvme_set_root(NULL);
        free(r);
 }