[PATCH v2 0/9] Some minor fixes/additions for nvme-cli

[PATCH v2 0/9] Some minor fixes/additions for nvme-cli

[mwilck]

From: Martin Wilck <mwilck@suse.com>

This series collects patches from my former submission "RFC: add "nvme
monitor" subcommand" which aren't directly related to the monitor
functionality, but required to make the monitor in its current form
compile. A v3 of the monitor code is about to follow.

The main part is the "generic logging functionality".

Changes wrt the RFC: add "nvme monitor" subcommand series: many.

 - rebased to current upstream code base e9d43dd ("[nvme-cli] Fix Max/Min 
   User data erase counts displayed in 0xC2 Log Page"))
 - renamed log() to msg() in order to prevent name clash with <math.h>
 - put logging and cleanup files in "util/" subdirectory
 - converted the bulk of the logging code from a macro into a function
   (Hannes)
 - added functionality to optionally log the PID (default off).
 - added Hannes' patch he recently submitted via github
   (https://github.com/linux-nvme/nvme-cli/pull/942), with two minor
   fixes on top, rationale in the commit messages
 - various additional fixes for memory handling

Both cleanup and logging functions / macros could be used in more
places with positive effects, but my focus is currently on preparing
the monitor functionality.

Changes v1 -> v2:

 - rebased on current upstream
 - squashed previous patches 1 and 2 into one, and simplified the logic
   (Sagi) - if the kernel has no "kato" attribute, we now simply assume
   that existing discovery controllers we encounter are persistent.
 - added Reviewed-by: trailers where appropriate

Hannes Reinecke / Martin Wilck (1):
  nvme-discover: lookup existing persistent controllers

Martin Wilck (8):
  do_discover: free cfg.device when resetting it
  nvme-connect-all(1): fix documentation for --quiet/-S
  nvme: add some simplifying macros for __attribute__((cleanup()))
  nvme-cli: add generic logging functionality
  nvme: convert some function arguments from "char *" to "const char *"
  fabrics: use "const char *" in struct config
  fabrics: fix some memory leaks
  fabrics: fix invalid memory access in discover_from_conf_file()

 Documentation/nvme-connect-all.1    |   8 +-
 Documentation/nvme-connect-all.html |  10 +-
 Documentation/nvme-connect-all.txt  |   4 +-
 Makefile                            |   3 +-
 fabrics.c                           | 218 +++++++++++++++++-----------
 nvme-topology.c                     |   2 +-
 nvme.c                              |   2 +-
 nvme.h                              |   4 +-
 util/cleanup.c                      |   4 +
 util/cleanup.h                      |  18 +++
 util/log.c                          |  90 ++++++++++++
 util/log.h                          |  34 +++++
 12 files changed, 295 insertions(+), 102 deletions(-)
 create mode 100644 util/cleanup.c
 create mode 100644 util/cleanup.h
 create mode 100644 util/log.c
 create mode 100644 util/log.h

-- 
2.30.1


_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

[PATCH v2 1/9] nvme-discover: lookup existing persistent controllers

[mwilck]

From: Hannes Reinecke <hare@suse.de>

If persistent controller connections are present they should be
preferred even if no --device option is given on the commandline.

To avoid selecting a temporary non-persistent controller the
'kato' attribute is checked; any controller for which the
attribute is '0' will be skipped. This logic is not applied on
older kernels that don't support the 'kato' attribute. On these
kernels, we just have to assume that the encountered discovery
controller is persistent.

Signed-off-by: Hannes Reinecke <hare@suse.de>
Signed-off-by: Martin Wilck <mwilck@suse.de>
Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 fabrics.c | 55 ++++++++++++++++++++++++++++++++-----------------------
 1 file changed, 32 insertions(+), 23 deletions(-)

diff --git a/fabrics.c b/fabrics.c
index 227773d..bdd0679 100644
--- a/fabrics.c
+++ b/fabrics.c
@@ -303,6 +303,7 @@ static bool ctrl_matches_connectargs(char *name, struct connect_args *args)
 	bool found = false;
 	char *path, *addr;
 	int ret;
+	bool persistent = true;
 
 	ret = asprintf(&path, "%s/%s", SYS_NVME, name);
 	if (ret < 0)
@@ -320,7 +321,30 @@ static bool ctrl_matches_connectargs(char *name, struct connect_args *args)
 	cargs.trsvcid = parse_conn_arg(addr, ' ', conarg_trsvcid);
 	cargs.host_traddr = parse_conn_arg(addr, ' ', conarg_host_traddr);
 
-	if (!strcmp(cargs.subsysnqn, args->subsysnqn) &&
+	if (!strcmp(cargs.subsysnqn, NVME_DISC_SUBSYS_NAME)) {
+		char *kato_str = nvme_get_ctrl_attr(path, "kato"), *p;
+		unsigned int kato = 0;
+
+		/*
+		 * When looking up discovery controllers we have to skip
+		 * any non-persistent controllers (ie those with a zero
+		 * kato value). Otherwise the controller will vanish from
+		 * underneath us as they are owned by another program.
+		 *
+		 * On older kernels, the 'kato' attribute isn't present.
+		 * Assume a persistent controller for these installations.
+		 */
+		if (kato_str) {
+			kato = strtoul(kato_str, &p, 0);
+			if (p == kato_str)
+				kato = 0;
+			free(kato_str);
+			persistent = (kato != 0);
+		}
+	}
+
+	if (persistent &&
+	    !strcmp(cargs.subsysnqn, args->subsysnqn) &&
 	    !strcmp(cargs.transport, args->transport) &&
 	    (!strcmp(cargs.traddr, args->traddr) ||
 	     !strcmp(args->traddr, "none")) &&
@@ -1346,30 +1370,15 @@ static int do_discover(char *argstr, bool connect, enum nvme_print_flags flags)
 	char *dev_name;
 	int instance, numrec = 0, ret, err;
 	int status = 0;
+	struct connect_args *cargs;
 
-	if (cfg.device) {
-		struct connect_args *cargs;
+	cargs = extract_connect_args(argstr);
+	if (!cargs)
+		return -ENOMEM;
 
-		cargs = extract_connect_args(argstr);
-		if (!cargs)
-			return -ENOMEM;
-
-		/*
-		 * if the cfg.device passed in matches the connect args
-		 *    cfg.device is left as-is
-		 * else if there exists a controller that matches the
-		 *         connect args
-		 *    cfg.device is the matching ctrl name
-		 * else if no ctrl matches the connect args
-		 *    cfg.device is set to null. This will attempt to
-		 *    create a new ctrl.
-		 * endif
-		 */
-		if (!ctrl_matches_connectargs(cfg.device, cargs))
-			cfg.device = find_ctrl_with_connectargs(cargs);
-
-		free_connect_args(cargs);
-	}
+	if (!cfg.device || !ctrl_matches_connectargs(cfg.device, cargs))
+		cfg.device = find_ctrl_with_connectargs(cargs);
+	free_connect_args(cargs);
 
 	if (!cfg.device) {
 		instance = add_ctrl(argstr);
-- 
2.30.1


_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

[PATCH v2 2/9] do_discover: free cfg.device when resetting it

[mwilck]

From: Martin Wilck <mwilck@suse.com>

cfg.device might have been allocated by a previous call to
find_ctrl_with_connectargs(), therefore free it. We must make sure
that cfg.device is always on the heap, thus change fabrics_discover()
accordingly.

Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 fabrics.c | 15 ++++++++++++---
 1 file changed, 12 insertions(+), 3 deletions(-)

diff --git a/fabrics.c b/fabrics.c
index bdd0679..49331cd 100644
--- a/fabrics.c
+++ b/fabrics.c
@@ -1376,7 +1376,11 @@ static int do_discover(char *argstr, bool connect, enum nvme_print_flags flags)
 	if (!cargs)
 		return -ENOMEM;
 
-	if (!cfg.device || !ctrl_matches_connectargs(cfg.device, cargs))
+	if (cfg.device && !ctrl_matches_connectargs(cfg.device, cargs)) {
+		free(cfg.device);
+		cfg.device = NULL;
+	}
+	if (!cfg.device)
 		cfg.device = find_ctrl_with_connectargs(cargs);
 	free_connect_args(cargs);
 
@@ -1560,8 +1564,13 @@ int fabrics_discover(const char *desc, int argc, char **argv, bool connect)
 	ret = flags = validate_output_format(cfg.output_format);
 	if (ret < 0)
 		goto out;
-	if (cfg.device && !strcmp(cfg.device, "none"))
-		cfg.device = NULL;
+	if (cfg.device && strcmp(cfg.device, "none")) {
+		cfg.device = strdup(cfg.device);
+		if (!cfg.device) {
+			ret = -ENOMEM;
+			goto out;
+		}
+	}
 
 	cfg.nqn = NVME_DISC_SUBSYS_NAME;
 
-- 
2.30.1


_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

[PATCH v2 3/9] nvme-connect-all(1): fix documentation for --quiet/-S

[mwilck]

From: Martin Wilck <mwilck@suse.com>

Reviewed-by: Chaitanya Kulkarni <chaitanya.kulkarni@wdc.com>
Reviewed-by: Sagi Grimberg <sagi@grimberg.me>
Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 Documentation/nvme-connect-all.1    |  8 ++++----
 Documentation/nvme-connect-all.html | 10 +++++-----
 Documentation/nvme-connect-all.txt  |  4 ++--
 3 files changed, 11 insertions(+), 11 deletions(-)

diff --git a/Documentation/nvme-connect-all.1 b/Documentation/nvme-connect-all.1
index 91d6896..a53a017 100644
--- a/Documentation/nvme-connect-all.1
+++ b/Documentation/nvme-connect-all.1
@@ -2,12 +2,12 @@
 .\"     Title: nvme-connect-all
 .\"    Author: [FIXME: author] [see http://www.docbook.org/tdg5/en/html/author]
 .\" Generator: DocBook XSL Stylesheets vsnapshot <http://docbook.sf.net/>
-.\"      Date: 10/20/2020
+.\"      Date: 01/20/2021
 .\"    Manual: NVMe Manual
 .\"    Source: NVMe
 .\"  Language: English
 .\"
-.TH "NVME\-CONNECT\-ALL" "1" "10/20/2020" "NVMe" "NVMe Manual"
+.TH "NVME\-CONNECT\-ALL" "1" "01/20/2021" "NVMe" "NVMe Manual"
 .\" -----------------------------------------------------------------
 .\" * Define some portability stuff
 .\" -----------------------------------------------------------------
@@ -51,7 +51,7 @@ nvme-connect-all \- Discover and Connect to Fabrics controllers\&.
                 [\-\-queue\-size=<#>         | \-Q <#>]
                 [\-\-matching               | \-m]
                 [\-\-persistent             | \-p]
-                [\-\-quiet                  | \-q]
+                [\-\-quiet                  | \-S]
 .fi
 .SH "DESCRIPTION"
 .sp
@@ -193,7 +193,7 @@ If a traddr was specified on the command line or in the configuration file, only
 Don\(cqt remove the discovery controller after retrieving the discovery log page\&.
 .RE
 .PP
-\-q, \-\-quiet
+\-S, \-\-quiet
 .RS 4
 Suppress error messages\&.
 .RE
diff --git a/Documentation/nvme-connect-all.html b/Documentation/nvme-connect-all.html
index beae013..f779c7b 100644
--- a/Documentation/nvme-connect-all.html
+++ b/Documentation/nvme-connect-all.html
@@ -4,7 +4,7 @@
 <html xmlns="http://www.w3.org/1999/xhtml" xml:lang="en">
 <head>
 <meta http-equiv="Content-Type" content="application/xhtml+xml; charset=UTF-8" />
-<meta name="generator" content="AsciiDoc 8.6.10" />
+<meta name="generator" content="AsciiDoc" />
 <title>nvme-connect-all(1)</title>
 <style type="text/css">
 /* Shared CSS for AsciiDoc xhtml11 and html5 backends */
@@ -436,7 +436,7 @@ thead, p.table.header {
 p.table {
   margin-top: 0;
 }
-/* Because the table frame attribute is overriden by CSS in most browsers. */
+/* Because the table frame attribute is overridden by CSS in most browsers. */
 div.tableblock > table[frame="void"] {
   border-style: none;
 }
@@ -768,7 +768,7 @@ nvme-connect-all(1) Manual Page
                 [--queue-size=&lt;#&gt;         | -Q &lt;#&gt;]
                 [--matching               | -m]
                 [--persistent             | -p]
-                [--quiet                  | -q]</pre>
+                [--quiet                  | -S]</pre>
 <div class="attribution">
 </div></div>
 </div>
@@ -1048,7 +1048,7 @@ cellspacing="0" cellpadding="4">
 </p>
 </dd>
 <dt class="hdlist1">
--q
+-S
 </dt>
 <dt class="hdlist1">
 --quiet
@@ -1115,7 +1115,7 @@ nvme-connect(1)</p></div>
 <div id="footer">
 <div id="footer-text">
 Last updated
- 2020-10-20 16:47:21 PDT
+ 2021-01-20 23:40:57 CET
 </div>
 </div>
 </body>
diff --git a/Documentation/nvme-connect-all.txt b/Documentation/nvme-connect-all.txt
index 128f336..820dd6c 100644
--- a/Documentation/nvme-connect-all.txt
+++ b/Documentation/nvme-connect-all.txt
@@ -27,7 +27,7 @@ SYNOPSIS
 		[--queue-size=<#>         | -Q <#>]
 		[--matching               | -m]
 		[--persistent             | -p]
-		[--quiet                  | -q]
+		[--quiet                  | -S]
 
 DESCRIPTION
 -----------
@@ -154,7 +154,7 @@ OPTIONS
 	Don't remove the discovery controller after retrieving the discovery
 	log page.
 
--q::
+-S::
 --quiet::
 	Suppress error messages.
 
-- 
2.30.1


_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

[PATCH v2 4/9] nvme: add some simplifying macros for __attribute__((cleanup()))

[mwilck]

From: Martin Wilck <mwilck@suse.com>

Using __attribute__((cleanup())) is very helpful for writing leak-free
code, but it requires lots of awkward boiler plate code. Add some
small helpers to make its use more comfortable.

Reviewed-by: Chaitanya Kulkarni <chaitanya.kulkarni@wdc.com>
Reviewed-by: Sagi Grimberg <sagi@grimberg.me>
Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 Makefile       |  2 +-
 util/cleanup.c |  4 ++++
 util/cleanup.h | 18 ++++++++++++++++++
 3 files changed, 23 insertions(+), 1 deletion(-)
 create mode 100644 util/cleanup.c
 create mode 100644 util/cleanup.h

diff --git a/Makefile b/Makefile
index 3ea17b5..3412e5d 100644
--- a/Makefile
+++ b/Makefile
@@ -62,7 +62,7 @@ OBJS := nvme-print.o nvme-ioctl.o nvme-rpmb.o \
 	nvme-lightnvm.o fabrics.o nvme-models.o plugin.o \
 	nvme-status.o nvme-filters.o nvme-topology.o
 
-UTIL_OBJS := util/argconfig.o util/suffix.o util/json.o util/parser.o
+UTIL_OBJS := util/argconfig.o util/suffix.o util/json.o util/parser.o util/cleanup.o
 
 PLUGIN_OBJS :=					\
 	plugins/intel/intel-nvme.o		\
diff --git a/util/cleanup.c b/util/cleanup.c
new file mode 100644
index 0000000..0d5d910
--- /dev/null
+++ b/util/cleanup.c
@@ -0,0 +1,4 @@
+#include <stdlib.h>
+#include "cleanup.h"
+
+DEFINE_CLEANUP_FUNC(cleanup_charp, char *, free);
diff --git a/util/cleanup.h b/util/cleanup.h
new file mode 100644
index 0000000..89a4984
--- /dev/null
+++ b/util/cleanup.h
@@ -0,0 +1,18 @@
+#ifndef __CLEANUP_H
+#define __CLEANUP_H
+
+#define __cleanup__(fn) __attribute__((cleanup(fn)))
+
+#define DECLARE_CLEANUP_FUNC(name, type) \
+	void name(type *__p)
+
+#define DEFINE_CLEANUP_FUNC(name, type, free_fn)\
+DECLARE_CLEANUP_FUNC(name, type)		\
+{						\
+	if (*__p)				\
+		free_fn(*__p);			\
+}
+
+DECLARE_CLEANUP_FUNC(cleanup_charp, char *);
+
+#endif
-- 
2.30.1


_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

[PATCH v2 5/9] nvme-cli: add generic logging functionality

[mwilck]

From: Martin Wilck <mwilck@suse.com>

Add a msg() macro that allows more flexible customization of logging
both at build time and at run time. Allow several log levels, using
the well-known standard sylog levels. Also optionally allow printing
of log timestamps.

Put '#define LOG_FUNCNAME' before '#include "util/log.h"' to enable printing
the name of the calling function before the log message.

Use this functionality in the fabrics code for now, wherever fprintf(stderr, ...)
had been used.

No functional change except changing the output channel of 554db7d ("print
device name when creating a persistent device") from stdout to stderr.

Reviewed-by: Chaitanya Kulkarni <chaitanya.kulkarni@wdc.com>
Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 Makefile   |  3 +-
 fabrics.c  | 85 ++++++++++++++++++++++++++++-----------------------
 util/log.c | 90 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
 util/log.h | 34 +++++++++++++++++++++
 4 files changed, 172 insertions(+), 40 deletions(-)
 create mode 100644 util/log.c
 create mode 100644 util/log.h

diff --git a/Makefile b/Makefile
index 3412e5d..0eaedb6 100644
--- a/Makefile
+++ b/Makefile
@@ -62,7 +62,8 @@ OBJS := nvme-print.o nvme-ioctl.o nvme-rpmb.o \
 	nvme-lightnvm.o fabrics.o nvme-models.o plugin.o \
 	nvme-status.o nvme-filters.o nvme-topology.o
 
-UTIL_OBJS := util/argconfig.o util/suffix.o util/json.o util/parser.o util/cleanup.o
+UTIL_OBJS := util/argconfig.o util/suffix.o util/json.o util/parser.o \
+	util/cleanup.o util/log.o
 
 PLUGIN_OBJS :=					\
 	plugins/intel/intel-nvme.o		\
diff --git a/fabrics.c b/fabrics.c
index 49331cd..1aa1507 100644
--- a/fabrics.c
+++ b/fabrics.c
@@ -32,6 +32,8 @@
 #include <libgen.h>
 #include <sys/stat.h>
 #include <stddef.h>
+#include <syslog.h>
+#include <time.h>
 
 #include <sys/types.h>
 #include <arpa/inet.h>
@@ -46,6 +48,7 @@
 #include "util/argconfig.h"
 
 #include "common.h"
+#include "util/log.h"
 
 #ifdef HAVE_SYSTEMD
 #include <systemd/sd-id128.h>
@@ -86,7 +89,6 @@ static struct config {
 	int  hdr_digest;
 	int  data_digest;
 	bool persistent;
-	bool quiet;
 	bool matching_only;
 	char *output_format;
 } cfg = {
@@ -378,7 +380,7 @@ static char *find_ctrl_with_connectargs(struct connect_args *args)
 
 	n = scandir(SYS_NVME, &devices, scan_ctrls_filter, alphasort);
 	if (n < 0) {
-		fprintf(stderr, "no NVMe controller(s) detected.\n");
+		msg(LOG_ERR, "no NVMe controller(s) detected.\n");
 		return NULL;
 	}
 
@@ -386,7 +388,7 @@ static char *find_ctrl_with_connectargs(struct connect_args *args)
 		if (ctrl_matches_connectargs(devices[i]->d_name, args)) {
 			devname = strdup(devices[i]->d_name);
 			if (devname == NULL)
-				fprintf(stderr, "no memory for ctrl name %s\n",
+				msg(LOG_ERR, "no memory for ctrl name %s\n",
 						devices[i]->d_name);
 			goto cleanup_devices;
 		}
@@ -448,7 +450,7 @@ static int add_ctrl(const char *argstr)
 
 	fd = open(PATH_NVME_FABRICS, O_RDWR);
 	if (fd < 0) {
-		fprintf(stderr, "Failed to open %s: %s\n",
+		msg(LOG_ERR, "Failed to open %s: %s\n",
 			 PATH_NVME_FABRICS, strerror(errno));
 		ret = -errno;
 		goto out;
@@ -456,8 +458,8 @@ static int add_ctrl(const char *argstr)
 
 	ret = write(fd, argstr, len);
 	if (ret != len) {
-		if (errno != EALREADY || !cfg.quiet)
-			fprintf(stderr, "Failed to write to %s: %s\n",
+		if (errno != EALREADY)
+			msg(LOG_NOTICE, "Failed to write to %s: %s\n",
 				 PATH_NVME_FABRICS, strerror(errno));
 		ret = -errno;
 		goto out_close;
@@ -465,7 +467,7 @@ static int add_ctrl(const char *argstr)
 
 	len = read(fd, buf, BUF_SIZE);
 	if (len < 0) {
-		fprintf(stderr, "Failed to read from %s: %s\n",
+		msg(LOG_ERR, "Failed to read from %s: %s\n",
 			 PATH_NVME_FABRICS, strerror(errno));
 		ret = -errno;
 		goto out_close;
@@ -492,7 +494,7 @@ static int add_ctrl(const char *argstr)
 	}
 
 out_fail:
-	fprintf(stderr, "Failed to parse ctrl info for \"%s\"\n", argstr);
+	msg(LOG_ERR, "Failed to parse ctrl info for \"%s\"\n", argstr);
 	ret = -EINVAL;
 out_close:
 	close(fd);
@@ -507,7 +509,7 @@ static int remove_ctrl_by_path(char *sysfs_path)
 	fd = open(sysfs_path, O_WRONLY);
 	if (fd < 0) {
 		ret = -errno;
-		fprintf(stderr, "Failed to open %s: %s\n", sysfs_path,
+		msg(LOG_ERR, "Failed to open %s: %s\n", sysfs_path,
 				strerror(errno));
 		goto out;
 	}
@@ -561,7 +563,7 @@ static int nvmf_get_log_page_discovery(const char *dev_path,
 	fd = open(dev_path, O_RDWR);
 	if (fd < 0) {
 		error = -errno;
-		fprintf(stderr, "Failed to open %s: %s\n",
+		msg(LOG_ERR, "Failed to open %s: %s\n",
 				dev_path, strerror(errno));
 		goto out;
 	}
@@ -786,7 +788,7 @@ static void save_discovery_log(struct nvmf_disc_rsp_page_hdr *log, int numrec)
 
 	fd = open(cfg.raw, O_CREAT|O_RDWR|O_TRUNC, S_IRUSR|S_IWUSR);
 	if (fd < 0) {
-		fprintf(stderr, "failed to open %s: %s\n",
+		msg(LOG_ERR, "failed to open %s: %s\n",
 			cfg.raw, strerror(errno));
 		return;
 	}
@@ -795,7 +797,7 @@ static void save_discovery_log(struct nvmf_disc_rsp_page_hdr *log, int numrec)
 			numrec * sizeof(struct nvmf_disc_rsp_page_entry);
 	ret = write(fd, log, len);
 	if (ret < 0)
-		fprintf(stderr, "failed to write to %s: %s\n",
+		msg(LOG_ERR, "failed to write to %s: %s\n",
 			cfg.raw, strerror(errno));
 	else
 		printf("Discovery log is saved to %s\n", cfg.raw);
@@ -942,13 +944,13 @@ static int build_options(char *argstr, int max_len, bool discover)
 	int len;
 
 	if (!cfg.transport) {
-		fprintf(stderr, "need a transport (-t) argument\n");
+		msg(LOG_ERR, "need a transport (-t) argument\n");
 		return -EINVAL;
 	}
 
 	if (strncmp(cfg.transport, "loop", 4)) {
 		if (!cfg.traddr) {
-			fprintf(stderr, "need a address (-a) argument\n");
+			msg(LOG_ERR, "need a address (-a) argument\n");
 			return -EINVAL;
 		}
 	}
@@ -1042,7 +1044,7 @@ static int hostname2traddr(struct config *cfg)
 
 	ret = getaddrinfo(cfg->traddr, NULL, &hints, &host_info);
 	if (ret) {
-		fprintf(stderr, "failed to resolve host %s info\n", cfg->traddr);
+		msg(LOG_ERR, "failed to resolve host %s info\n", cfg->traddr);
 		return ret;
 	}
 
@@ -1058,14 +1060,14 @@ static int hostname2traddr(struct config *cfg)
 			addrstr, NVMF_TRADDR_SIZE);
 		break;
 	default:
-		fprintf(stderr, "unrecognized address family (%d) %s\n",
+		msg(LOG_ERR, "unrecognized address family (%d) %s\n",
 			host_info->ai_family, cfg->traddr);
 		ret = -EINVAL;
 		goto free_addrinfo;
 	}
 
 	if (!p) {
-		fprintf(stderr, "failed to get traddr for %s\n", cfg->traddr);
+		msg(LOG_ERR, "failed to get traddr for %s\n", cfg->traddr);
 		ret = -errno;
 		goto free_addrinfo;
 	}
@@ -1093,7 +1095,7 @@ retry:
 	case NVME_NQN_NVME:
 		break;
 	default:
-		fprintf(stderr, "skipping unsupported subtype %d\n",
+		msg(LOG_ERR, "skipping unsupported subtype %d\n",
 			 e->subtype);
 		return -EINVAL;
 	}
@@ -1182,7 +1184,7 @@ retry:
 
 	transport = trtype_str(e->trtype);
 	if (!strcmp(transport, "unrecognized")) {
-		fprintf(stderr, "skipping unsupported transport %d\n",
+		msg(LOG_ERR, "skipping unsupported transport %d\n",
 				 e->trtype);
 		return -EINVAL;
 	}
@@ -1228,7 +1230,7 @@ retry:
 			p += len;
 			break;
 		default:
-			fprintf(stderr, "skipping unsupported adrfam\n");
+			msg(LOG_ERR, "skipping unsupported adrfam\n");
 			return -EINVAL;
 		}
 		break;
@@ -1243,7 +1245,7 @@ retry:
 			p += len;
 			break;
 		default:
-			fprintf(stderr, "skipping unsupported adrfam\n");
+			msg(LOG_ERR, "skipping unsupported adrfam\n");
 			return -EINVAL;
 		}
 		break;
@@ -1334,9 +1336,7 @@ static int connect_ctrls(struct nvmf_disc_rsp_page_hdr *log, int numrec)
 		if (instance == -EALREADY) {
 			const char *traddr = log->entries[i].traddr;
 
-			if (!cfg.quiet)
-				fprintf(stderr,
-					"traddr=%.*s is already connected\n",
+			msg(LOG_NOTICE, "traddr=%.*s is already connected\n",
 					space_strip_len(NVMF_TRADDR_SIZE,
 							traddr),
 					traddr);
@@ -1398,7 +1398,7 @@ static int do_discover(char *argstr, bool connect, enum nvme_print_flags flags)
 	ret = nvmf_get_log_page_discovery(dev_name, &log, &numrec, &status);
 	free(dev_name);
 	if (cfg.persistent)
-		printf("Persistent device: nvme%d\n", instance);
+		msg(LOG_NOTICE, "Persistent device: nvme%d\n", instance);
 	if (!cfg.device && !cfg.persistent) {
 		err = remove_ctrl(instance);
 		if (err)
@@ -1417,12 +1417,12 @@ static int do_discover(char *argstr, bool connect, enum nvme_print_flags flags)
 			print_discovery_log(log, numrec);
 		break;
 	case DISC_GET_NUMRECS:
-		fprintf(stderr,
+		msg(LOG_ERR,
 			"Get number of discovery log entries failed.\n");
 		ret = status;
 		break;
 	case DISC_GET_LOG:
-		fprintf(stderr, "Get discovery log entries failed.\n");
+		msg(LOG_ERR, "Get discovery log entries failed.\n");
 		ret = status;
 		break;
 	case DISC_NO_LOG:
@@ -1434,12 +1434,12 @@ static int do_discover(char *argstr, bool connect, enum nvme_print_flags flags)
 		ret = -EAGAIN;
 		break;
 	case DISC_NOT_EQUAL:
-		fprintf(stderr,
+		msg(LOG_ERR,
 		"Numrec values of last two get discovery log page not equal\n");
 		ret = -EBADSLT;
 		break;
 	default:
-		fprintf(stderr, "Get discovery log page failed: %d\n", ret);
+		msg(LOG_ERR, "Get discovery log page failed: %d\n", ret);
 		break;
 	}
 
@@ -1455,7 +1455,7 @@ static int discover_from_conf_file(const char *desc, char *argstr,
 
 	f = fopen(PATH_NVMF_DISC, "r");
 	if (f == NULL) {
-		fprintf(stderr, "No discover params given and no %s\n",
+		msg(LOG_ERR, "No discover params given and no %s\n",
 			PATH_NVMF_DISC);
 		return -EINVAL;
 	}
@@ -1468,14 +1468,14 @@ static int discover_from_conf_file(const char *desc, char *argstr,
 
 		args = strdup(line);
 		if (!args) {
-			fprintf(stderr, "failed to strdup args\n");
+			msg(LOG_ERR, "failed to strdup args\n");
 			ret = -ENOMEM;
 			goto out;
 		}
 
 		argv = calloc(MAX_DISC_ARGS, BUF_SIZE);
 		if (!argv) {
-			perror("failed to allocate argv vector\n");
+			msg(LOG_ERR, "failed to allocate argv vector: %m\n");
 			free(args);
 			ret = -ENOMEM;
 			goto out;
@@ -1529,6 +1529,7 @@ int fabrics_discover(const char *desc, int argc, char **argv, bool connect)
 	char argstr[BUF_SIZE];
 	int ret;
 	enum nvme_print_flags flags;
+	bool quiet = false;
 
 	OPT_ARGS(opts) = {
 		OPT_LIST("transport",      't', &cfg.transport,       "transport type"),
@@ -1550,7 +1551,7 @@ int fabrics_discover(const char *desc, int argc, char **argv, bool connect)
 		OPT_INT("nr-poll-queues",  'P', &cfg.nr_poll_queues,  "number of poll queues to use (default 0)"),
 		OPT_INT("queue-size",      'Q', &cfg.queue_size,      "number of io queue elements to use (default 128)"),
 		OPT_FLAG("persistent",     'p', &cfg.persistent,      "persistent discovery connection"),
-		OPT_FLAG("quiet",          'S', &cfg.quiet,           "suppress already connected errors"),
+		OPT_FLAG("quiet",          'S', &quiet,               "suppress already connected errors"),
 		OPT_FLAG("matching",       'm', &cfg.matching_only,   "connect only records matching the traddr"),
 		OPT_FMT("output-format",   'o', &cfg.output_format,   output_format),
 		OPT_END()
@@ -1572,6 +1573,12 @@ int fabrics_discover(const char *desc, int argc, char **argv, bool connect)
 		}
 	}
 
+	if (quiet)
+		log_level = LOG_WARNING;
+
+	if (cfg.device && !strcmp(cfg.device, "none"))
+		cfg.device = NULL;
+
 	cfg.nqn = NVME_DISC_SUBSYS_NAME;
 
 	if (!cfg.transport && !cfg.traddr) {
@@ -1643,7 +1650,7 @@ int fabrics_connect(const char *desc, int argc, char **argv)
 		goto out;
 
 	if (!cfg.nqn) {
-		fprintf(stderr, "need a -n argument\n");
+		msg(LOG_ERR, "need a -n argument\n");
 		ret = -EINVAL;
 		goto out;
 	}
@@ -1681,7 +1688,7 @@ static int disconnect_subsys(char *nqn, char *ctrl)
 
 	fd = open(sysfs_nqn_path, O_RDONLY);
 	if (fd < 0) {
-		fprintf(stderr, "Failed to open %s: %s\n",
+		msg(LOG_ERR, "Failed to open %s: %s\n",
 				sysfs_nqn_path, strerror(errno));
 		goto free;
 	}
@@ -1755,7 +1762,7 @@ int fabrics_disconnect(const char *desc, int argc, char **argv)
 		goto out;
 
 	if (!cfg.nqn && !cfg.device) {
-		fprintf(stderr, "need a -n or -d argument\n");
+		msg(LOG_ERR, "need a -n or -d argument\n");
 		ret = -EINVAL;
 		goto out;
 	}
@@ -1763,7 +1770,7 @@ int fabrics_disconnect(const char *desc, int argc, char **argv)
 	if (cfg.nqn) {
 		ret = disconnect_by_nqn(cfg.nqn);
 		if (ret < 0)
-			fprintf(stderr, "Failed to disconnect by NQN: %s\n",
+			msg(LOG_ERR, "Failed to disconnect by NQN: %s\n",
 				cfg.nqn);
 		else {
 			printf("NQN:%s disconnected %d controller(s)\n", cfg.nqn, ret);
@@ -1774,7 +1781,7 @@ int fabrics_disconnect(const char *desc, int argc, char **argv)
 	if (cfg.device) {
 		ret = disconnect_by_device(cfg.device);
 		if (ret)
-			fprintf(stderr,
+			msg(LOG_ERR,
 				"Failed to disconnect by device name: %s\n",
 				cfg.device);
 	}
@@ -1798,7 +1805,7 @@ int fabrics_disconnect_all(const char *desc, int argc, char **argv)
 
 	err = scan_subsystems(&t, NULL, 0, 0, NULL);
 	if (err) {
-		fprintf(stderr, "Failed to scan namespaces\n");
+		msg(LOG_ERR, "Failed to scan namespaces\n");
 		goto out;
 	}
 
diff --git a/util/log.c b/util/log.c
new file mode 100644
index 0000000..4a22354
--- /dev/null
+++ b/util/log.c
@@ -0,0 +1,90 @@
+/*
+ * Copyright (C) 2021 SUSE LLC
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License version
+ * 2 as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+ *
+ * This file implements basic logging functionality.
+ */
+
+#include <sys/types.h>
+#include <stdio.h>
+#include <stdlib.h>
+#include <stdarg.h>
+#include <stdbool.h>
+#include <syslog.h>
+#include <unistd.h>
+#include <time.h>
+#define LOG_FUNCNAME 1
+#include "log.h"
+#include "cleanup.h"
+
+#ifndef LOG_CLOCK
+#define LOG_CLOCK CLOCK_MONOTONIC
+#endif
+
+int log_level = DEFAULT_LOGLEVEL;
+bool log_timestamp;
+bool log_pid;
+
+void __attribute__((format(printf, 3, 4)))
+__msg(int lvl, const char *func, const char *format, ...)
+{
+	va_list ap;
+	char pidbuf[16];
+	char timebuf[32];
+	static const char *const formats[] = {
+		"%s%s%s",
+		"%s%s%s: ",
+		"%s<%s>%s ",
+		"%s<%s> %s: ",
+		"[%s] %s%s ",
+		"[%s]%s %s: ",
+		"[%s] <%s>%s ",
+		"[%s] <%s> %s: ",
+	};
+	char *header __cleanup__(cleanup_charp) = NULL;
+	char *message __cleanup__(cleanup_charp) = NULL;
+	int idx;
+
+	if (lvl > log_level)
+		return;
+
+	if (log_timestamp) {
+		struct timespec now;
+
+		clock_gettime(LOG_CLOCK, &now);
+		snprintf(timebuf, sizeof(timebuf), "%6ld.%06ld",
+			 (long)now.tv_sec, now.tv_nsec / 1000);
+	} else
+		*timebuf = '\0';
+
+	if (log_pid)
+		snprintf(pidbuf, sizeof(pidbuf), "%ld", (long)getpid());
+	else
+		*pidbuf = '\0';
+
+	idx = ((log_timestamp ? 1 : 0) << 2) |
+		((log_pid ? 1 : 0) << 1) | (func ? 1 : 0);
+
+	if (asprintf(&header, formats[idx], timebuf, pidbuf, func ? func : "")
+	    == -1)
+		header = NULL;
+
+	va_start(ap, format);
+	if (vasprintf(&message, format, ap) == -1)
+		message = NULL;
+	va_end(ap);
+
+	fprintf(stderr, "%s%s", header ? header : "<error>",
+		message ? message : "<error>");
+
+}
diff --git a/util/log.h b/util/log.h
new file mode 100644
index 0000000..15107a5
--- /dev/null
+++ b/util/log.h
@@ -0,0 +1,34 @@
+/*
+ * Copyright (c) 2021 Martin Wilck, SUSE LLC
+ * SPDX-License-Identifier: LGPL-2.1-or-newer
+ */
+#ifndef _LOG_H
+#define _LOG_H
+
+#ifndef MAX_LOGLEVEL
+#  define MAX_LOGLEVEL LOG_DEBUG
+#endif
+#ifndef DEFAULT_LOGLEVEL
+#  define DEFAULT_LOGLEVEL LOG_NOTICE
+#endif
+
+#if (LOG_FUNCNAME == 1)
+#define _log_func __func__
+#else
+#define _log_func NULL
+#endif
+
+extern int log_level;
+extern bool log_timestamp;
+extern bool log_pid;
+
+void __attribute__((format(printf, 3, 4)))
+__msg(int lvl, const char *func, const char *format, ...);
+
+#define msg(lvl, format, ...)						\
+	do {								\
+		if ((lvl) <= MAX_LOGLEVEL)				\
+			__msg(lvl, _log_func, format, ##__VA_ARGS__);	\
+	} while (0)
+
+#endif /* _LOG_H */
-- 
2.30.1


_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

[PATCH v2 6/9] nvme: convert some function arguments from "char *" to "const char *"

[mwilck]

From: Martin Wilck <mwilck@suse.com>

Make some function arguments "const char *" where possible. This
is generally desirable and allows passing string constants to these
function.

Reviewed-by: Chaitanya Kulkarni <chaitanya.kulkarni@wdc.com>
Reviewed-by: Sagi Grimberg <sagi@grimberg.me>
Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 fabrics.c       | 25 +++++++++++++++----------
 nvme-topology.c |  2 +-
 nvme.c          |  2 +-
 nvme.h          |  4 ++--
 4 files changed, 19 insertions(+), 14 deletions(-)

diff --git a/fabrics.c b/fabrics.c
index 1aa1507..4a5c2f9 100644
--- a/fabrics.c
+++ b/fabrics.c
@@ -234,7 +234,7 @@ static int do_discover(char *argstr, bool connect, enum nvme_print_flags flags);
  * If field found, return string containing field value. If field
  * not found, return an empty string.
  */
-static char *parse_conn_arg(char *conargs, const char delim, const char *field)
+static char *parse_conn_arg(const char *conargs, const char delim, const char *field)
 {
 	char *s, *e;
 	size_t cnt;
@@ -278,17 +278,22 @@ empty_field:
 	return strdup("\0");
 }
 
-static int ctrl_instance(char *device)
+static int ctrl_instance(const char *device)
 {
 	char d[64];
+	const char *p;
 	int ret, instance;
 
-	device = basename(device);
-	ret = sscanf(device, "nvme%d", &instance);
+	p = strrchr(device, '/');
+	if (p == NULL)
+		p = device;
+	else
+		p++;
+	ret = sscanf(p, "nvme%d", &instance);
 	if (ret <= 0)
 		return -EINVAL;
 	if (snprintf(d, sizeof(d), "nvme%d", instance) <= 0 ||
-	    strcmp(device, d))
+	    strcmp(p, d))
 		return -EINVAL;
 	return instance;
 }
@@ -299,7 +304,7 @@ static int ctrl_instance(char *device)
  * given.
  * Return true/false based on whether it matches
  */
-static bool ctrl_matches_connectargs(char *name, struct connect_args *args)
+static bool ctrl_matches_connectargs(const char *name, struct connect_args *args)
 {
 	struct connect_args cargs;
 	bool found = false;
@@ -924,7 +929,7 @@ add_int_argument(char **argstr, int *max_len, char *arg_str, int arg,
 }
 
 static int
-add_argument(char **argstr, int *max_len, char *arg_str, char *arg)
+add_argument(char **argstr, int *max_len, char *arg_str, const char *arg)
 {
 	int len;
 
@@ -1675,7 +1680,7 @@ static int scan_sys_nvme_filter(const struct dirent *d)
 /*
  * Returns 1 if disconnect occurred, 0 otherwise.
  */
-static int disconnect_subsys(char *nqn, char *ctrl)
+static int disconnect_subsys(const char *nqn, char *ctrl)
 {
 	char *sysfs_nqn_path = NULL, *sysfs_del_path = NULL;
 	char subsysnqn[NVMF_NQN_SIZE] = {};
@@ -1713,7 +1718,7 @@ static int disconnect_subsys(char *nqn, char *ctrl)
 /*
  * Returns the number of controllers successfully disconnected.
  */
-static int disconnect_by_nqn(char *nqn)
+static int disconnect_by_nqn(const char *nqn)
 {
 	struct dirent **devices = NULL;
 	int i, n, ret = 0;
@@ -1735,7 +1740,7 @@ static int disconnect_by_nqn(char *nqn)
 	return ret;
 }
 
-static int disconnect_by_device(char *device)
+static int disconnect_by_device(const char *device)
 {
 	int instance;
 
diff --git a/nvme-topology.c b/nvme-topology.c
index 63e433d..c2c9b38 100644
--- a/nvme-topology.c
+++ b/nvme-topology.c
@@ -46,7 +46,7 @@ close_fd:
 	return subsysnqn;
 }
 
-char *nvme_get_ctrl_attr(char *path, const char *attr)
+char *nvme_get_ctrl_attr(const char *path, const char *attr)
 {
 	char *attrpath, *value;
 	ssize_t ret;
diff --git a/nvme.c b/nvme.c
index 8751ffd..bf18366 100644
--- a/nvme.c
+++ b/nvme.c
@@ -209,7 +209,7 @@ int parse_and_open(int argc, char **argv, const char *desc,
 	return ret;
 }
 
-enum nvme_print_flags validate_output_format(char *format)
+enum nvme_print_flags validate_output_format(const char *format)
 {
 	if (!format)
 		return -EINVAL;
diff --git a/nvme.h b/nvme.h
index 8501754..1293d4d 100644
--- a/nvme.h
+++ b/nvme.h
@@ -91,7 +91,7 @@ int parse_and_open(int argc, char **argv, const char *desc,
 extern const char *devicename;
 extern const char *output_format;
 
-enum nvme_print_flags validate_output_format(char *format);
+enum nvme_print_flags validate_output_format(const char *format);
 int __id_ctrl(int argc, char **argv, struct command *cmd,
 	struct plugin *plugin, void (*vs)(__u8 *vs, struct json_object *root));
 char *nvme_char_from_block(char *block);
@@ -109,7 +109,7 @@ int scan_subsystems(struct nvme_topology *t, const char *subsysnqn,
 		    __u32 ns_instance, int nsid, char *dev_dir);
 void free_topology(struct nvme_topology *t);
 char *get_nvme_subsnqn(char *path);
-char *nvme_get_ctrl_attr(char *path, const char *attr);
+char *nvme_get_ctrl_attr(const char *path, const char *attr);
 
 void *nvme_alloc(size_t len, bool *huge);
 void nvme_free(void *p, bool huge);
-- 
2.30.1


_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

[PATCH v2 7/9] fabrics: use "const char *" in struct config

[mwilck]

From: Martin Wilck <mwilck@suse.com>

This is easily done, and allows passing in some const char* pointers
that would otherwise need to be strdup()d first.

Do not use const char * for cfg->device, because we may need to free()
it.

Reviewed-by: Chaitanya Kulkarni <chaitanya.kulkarni@wdc.com>
Reviewed-by: Sagi Grimberg <sagi@grimberg.me>
Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 fabrics.c | 18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)

diff --git a/fabrics.c b/fabrics.c
index 4a5c2f9..9a694bd 100644
--- a/fabrics.c
+++ b/fabrics.c
@@ -67,13 +67,13 @@ const char *conarg_trsvcid = "trsvcid";
 const char *conarg_host_traddr = "host_traddr";
 
 static struct config {
-	char *nqn;
-	char *transport;
-	char *traddr;
-	char *trsvcid;
-	char *host_traddr;
-	char *hostnqn;
-	char *hostid;
+	const char *nqn;
+	const char *transport;
+	const char *traddr;
+	const char *trsvcid;
+	const char *host_traddr;
+	const char *hostnqn;
+	const char *hostid;
 	int  nr_io_queues;
 	int  nr_write_queues;
 	int  nr_poll_queues;
@@ -82,7 +82,7 @@ static struct config {
 	int  reconnect_delay;
 	int  ctrl_loss_tmo;
 	int  tos;
-	char *raw;
+	const char *raw;
 	char *device;
 	int  duplicate_connect;
 	int  disable_sqflow;
@@ -90,7 +90,7 @@ static struct config {
 	int  data_digest;
 	bool persistent;
 	bool matching_only;
-	char *output_format;
+	const char *output_format;
 } cfg = {
 	.ctrl_loss_tmo = NVMF_DEF_CTRL_LOSS_TMO,
 	.output_format = "normal",
-- 
2.30.1


_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

[PATCH v2 8/9] fabrics: fix some memory leaks

[mwilck]

From: Martin Wilck <mwilck@suse.com>

None of these are critical for "nvme discover" or "nvme connect-all".
Still, silencing valgrind's error messages by fixing them gives some
peace of mind, and a longer-running program like the forthcomint
nvme monitor, leak checks are more important.

Use the previously introduced cleanup macros for this purpose.

Reviewed-by: Chaitanya Kulkarni <chaitanya.kulkarni@wdc.com>
Reviewed-by: Sagi Grimberg <sagi@grimberg.me>
Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 fabrics.c | 23 +++++++++++++++++------
 1 file changed, 17 insertions(+), 6 deletions(-)

diff --git a/fabrics.c b/fabrics.c
index 9a694bd..f9e5dc6 100644
--- a/fabrics.c
+++ b/fabrics.c
@@ -49,6 +49,7 @@
 
 #include "common.h"
 #include "util/log.h"
+#include "util/cleanup.h"
 
 #ifdef HAVE_SYSTEMD
 #include <systemd/sd-id128.h>
@@ -308,7 +309,7 @@ static bool ctrl_matches_connectargs(const char *name, struct connect_args *args
 {
 	struct connect_args cargs;
 	bool found = false;
-	char *path, *addr;
+	char *path = NULL, *addr;
 	int ret;
 	bool persistent = true;
 
@@ -366,6 +367,8 @@ static bool ctrl_matches_connectargs(const char *name, struct connect_args *args
 	free(cargs.traddr);
 	free(cargs.trsvcid);
 	free(cargs.host_traddr);
+	free(addr);
+	free(path);
 
 	return found;
 }
@@ -422,13 +425,18 @@ static struct connect_args *extract_connect_args(char *argstr)
 	return cargs;
 }
 
-static void free_connect_args(struct connect_args *cargs)
+static void destruct_connect_args(struct connect_args *cargs)
 {
 	free(cargs->subsysnqn);
 	free(cargs->transport);
 	free(cargs->traddr);
 	free(cargs->trsvcid);
 	free(cargs->host_traddr);
+}
+
+static void free_connect_args(struct connect_args *cargs)
+{
+	destruct_connect_args(cargs);
 	free(cargs);
 }
 
@@ -1283,7 +1291,7 @@ retry:
 
 static bool cargs_match_found(struct nvmf_disc_rsp_page_entry *entry)
 {
-	struct connect_args cargs = {};
+	struct connect_args cargs __cleanup__(destruct_connect_args) = { NULL, };
 	struct connect_args *c = tracked_ctrls;
 
 	cargs.traddr = strdup(entry->traddr);
@@ -1369,9 +1377,11 @@ static void nvmf_get_host_identifiers(int ctrl_instance)
 	cfg.hostid = nvme_get_ctrl_attr(path, "hostid");
 }
 
+static DEFINE_CLEANUP_FUNC(cleanup_log, struct nvmf_disc_rsp_page_hdr *, free);
+
 static int do_discover(char *argstr, bool connect, enum nvme_print_flags flags)
 {
-	struct nvmf_disc_rsp_page_hdr *log = NULL;
+	struct nvmf_disc_rsp_page_hdr *log __cleanup__(cleanup_log) = NULL;
 	char *dev_name;
 	int instance, numrec = 0, ret, err;
 	int status = 0;
@@ -1455,7 +1465,7 @@ static int discover_from_conf_file(const char *desc, char *argstr,
 		const struct argconfig_commandline_options *opts, bool connect)
 {
 	FILE *f;
-	char line[256], *ptr, *args, **argv;
+	char line[256], *ptr, *all_args, *args, **argv;
 	int argc, err, ret = 0;
 
 	f = fopen(PATH_NVMF_DISC, "r");
@@ -1477,6 +1487,7 @@ static int discover_from_conf_file(const char *desc, char *argstr,
 			ret = -ENOMEM;
 			goto out;
 		}
+		all_args = args;
 
 		argv = calloc(MAX_DISC_ARGS, BUF_SIZE);
 		if (!argv) {
@@ -1520,7 +1531,7 @@ static int discover_from_conf_file(const char *desc, char *argstr,
 			ret = err;
 
 free_and_continue:
-		free(args);
+		free(all_args);
 		free(argv);
 	}
 
-- 
2.30.1


_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

[PATCH v2 9/9] fabrics: fix invalid memory access in discover_from_conf_file()

[mwilck]

From: Martin Wilck <mwilck@suse.com>

argconfig_parse() assigns pointers in cfg to point to memory allocated
in all_args. If this memory is freed, these pointers become dangling.
This is particularly dangerous if discovery.conf contains empty lines,
comment lines, or invalid lines.

Fix it by setting all transport parameter to NULL after processing each
line, and not proceeding if the basic parameters aren't set.

Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 fabrics.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/fabrics.c b/fabrics.c
index f9e5dc6..c9324bb 100644
--- a/fabrics.c
+++ b/fabrics.c
@@ -1506,6 +1506,9 @@ static int discover_from_conf_file(const char *desc, char *argstr,
 		if (err)
 			goto free_and_continue;
 
+		if (!cfg.transport || !cfg.traddr)
+			goto free_and_continue;
+
 		err = flags = validate_output_format(cfg.output_format);
 		if (err < 0)
 			goto free_and_continue;
@@ -1533,6 +1536,8 @@ static int discover_from_conf_file(const char *desc, char *argstr,
 free_and_continue:
 		free(all_args);
 		free(argv);
+		cfg.transport = cfg.traddr = cfg.trsvcid =
+			cfg.host_traddr = NULL;
 	}
 
 out:
-- 
2.30.1


_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme