[PATCH 00/10] Some minor fixes/additions for nvme-cli

[PATCH 00/10] 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 v2 of the monitor code is about to follow soon.

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.

I've pushed this series https://github.com/linux-nvme/nvme-cli/pull/877,
together with the follow-up monitor series.

Regards
Martin

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

Martin Wilck (9):
  nvme-discover: assume device given on command line is persistent
  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                            |   2 +-
 fabrics.c                           | 223 +++++++++++++++++-----------
 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, 298 insertions(+), 103 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.29.2


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

[PATCH 01/10] 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 which does not have
this attribute or for which the attribute is '0' will be skipped.

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

diff --git a/fabrics.c b/fabrics.c
index f84c45b..14899cc 100644
--- a/fabrics.c
+++ b/fabrics.c
@@ -320,6 +320,27 @@ 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, 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.
+		 *
+		 * The 'kato' attribute might not be present; assume a
+		 * non-persistent controller for these installations.
+		 */
+		if (kato_str) {
+			kato = strtoul(kato_str, &p, 0);
+			if (p == kato_str)
+				kato = 0;
+		}
+		if (kato == 0)
+			return found;
+	}
 	if (!strcmp(cargs.subsysnqn, args->subsysnqn) &&
 	    !strcmp(cargs.transport, args->transport) &&
 	    (!strcmp(cargs.traddr, args->traddr) ||
@@ -1345,30 +1366,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.29.2


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

Re: [PATCH 01/10] nvme-discover: lookup existing persistent controllers

[Sagi Grimberg]

> 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 which does not have
> this attribute or for which the attribute is '0' will be skipped.

Why?

You also need a fallback in case kato is not exposed in prior
kernels.

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

Re: [PATCH 01/10] nvme-discover: lookup existing persistent controllers

[Martin Wilck]

On Mon, 2021-03-15 at 10:38 -0700, Sagi Grimberg wrote:
> 
> > 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 which does not have
> > this attribute or for which the attribute is '0' will be skipped.
> 
> Why?

The idea is that an existing non-persistent controller is likely
controller by another process, and could be disconnected any time.

> 
> You also need a fallback in case kato is not exposed in prior
> kernels.
> 

Agreed. That's handled in patch 02/10. I didn't want to modify Hannes'
patch directly.

Martin



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

Re: [PATCH 01/10] nvme-discover: lookup existing persistent controllers

[Hannes Reinecke]

On 3/15/21 6:38 PM, Sagi Grimberg wrote:
> 
>> 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 which does not have
>> this attribute or for which the attribute is '0' will be skipped.
> 
> Why?
> 
> You also need a fallback in case kato is not exposed in prior
> kernels.

The idea was to make the '--device' attribute optional _iff_ the kernel 
exposes the 'kato' attribute _and_ that attribute is non-zero.

Nothing more, nothing less.

Apparently the comment was slightly misleading.
I never intended to change the current behaviour if the 'kato' attribute 
is not exposed.

Cheers

Hannes
-- 
Dr. Hannes Reinecke                Kernel Storage Architect
hare@suse.de                              +49 911 74053 688
SUSE Software Solutions GmbH, Maxfeldstr. 5, 90409 Nürnberg
HRB 36809 (AG Nürnberg), Geschäftsführer: Felix Imendörffer

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

[PATCH 02/10] nvme-discover: assume device given on command line is persistent

[mwilck]

From: Martin Wilck <mwilck@suse.com>

After commit "nvme-discover: lookup existing persistent controllers",
controllers without the "kato" sysfs attribute will never be used by
do_discover(). This makes sense for controllers found while traversing
sysfs in find_ctrl_with_connectargs(), but if the user passed a
device explicitly, it should be used, even on older kernels that
don't support the "kato" attribute.

Furthermore, make sure allocated memory in ctrl_matches_connectargs()
is freed.

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

diff --git a/fabrics.c b/fabrics.c
index 14899cc..dcee0c4 100644
--- a/fabrics.c
+++ b/fabrics.c
@@ -297,12 +297,14 @@ 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(char *name, struct connect_args *args,
+				     bool assume_persistent)
 {
 	struct connect_args cargs;
 	bool found = false;
 	char *path, *addr;
 	int ret;
+	bool persistent = true;
 
 	ret = asprintf(&path, "%s/%s", SYS_NVME, name);
 	if (ret < 0)
@@ -331,17 +333,21 @@ static bool ctrl_matches_connectargs(char *name, struct connect_args *args)
 		 * underneath us as they are owned by another program.
 		 *
 		 * The 'kato' attribute might not be present; assume a
-		 * non-persistent controller for these installations.
+		 * non-persistent controller for these installations,
+		 * unless assume_persistent is set.
 		 */
 		if (kato_str) {
 			kato = strtoul(kato_str, &p, 0);
 			if (p == kato_str)
 				kato = 0;
-		}
-		if (kato == 0)
-			return found;
+			free(kato_str);
+			persistent = (kato != 0);
+		} else if (!assume_persistent)
+			persistent = false;
 	}
-	if (!strcmp(cargs.subsysnqn, args->subsysnqn) &&
+
+	if (persistent &&
+	    !strcmp(cargs.subsysnqn, args->subsysnqn) &&
 	    !strcmp(cargs.transport, args->transport) &&
 	    (!strcmp(cargs.traddr, args->traddr) ||
 	     !strcmp(args->traddr, "none")) &&
@@ -380,7 +386,7 @@ static char *find_ctrl_with_connectargs(struct connect_args *args)
 	}
 
 	for (i = 0; i < n; i++) {
-		if (ctrl_matches_connectargs(devices[i]->d_name, args)) {
+		if (ctrl_matches_connectargs(devices[i]->d_name, args, false)) {
 			devname = strdup(devices[i]->d_name);
 			if (devname == NULL)
 				fprintf(stderr, "no memory for ctrl name %s\n",
@@ -1372,7 +1378,7 @@ 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, true))
 		cfg.device = find_ctrl_with_connectargs(cargs);
 	free_connect_args(cargs);
 
-- 
2.29.2


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

Re: [PATCH 02/10] nvme-discover: assume device given on command line is persistent

[Sagi Grimberg]

> From: Martin Wilck <mwilck@suse.com>
> 
> After commit "nvme-discover: lookup existing persistent controllers",
> controllers without the "kato" sysfs attribute will never be used by
> do_discover(). This makes sense for controllers found while traversing
> sysfs in find_ctrl_with_connectargs(), but if the user passed a
> device explicitly, it should be used, even on older kernels that
> don't support the "kato" attribute.
> 
> Furthermore, make sure allocated memory in ctrl_matches_connectargs()
> is freed.

This is getting slightly convoluted... what is the motivation again?

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

Re: [PATCH 02/10] nvme-discover: assume device given on command line is persistent

[Martin Wilck]

On Mon, 2021-03-15 at 10:41 -0700, Sagi Grimberg wrote:
> 
> > From: Martin Wilck <mwilck@suse.com>
> > 
> > After commit "nvme-discover: lookup existing persistent
> > controllers",
> > controllers without the "kato" sysfs attribute will never be used
> > by
> > do_discover(). This makes sense for controllers found while
> > traversing
> > sysfs in find_ctrl_with_connectargs(), but if the user passed a
> > device explicitly, it should be used, even on older kernels that
> > don't support the "kato" attribute.
> > 
> > Furthermore, make sure allocated memory in
> > ctrl_matches_connectargs()
> > is freed.
> 
> This is getting slightly convoluted... what is the motivation again?
> 

Currently, users need to specify *both* connect args *and* a discovery
controller device if they want to reuse an existing discovery
controller, which doesn't make much sense from a usability point of
view. The idea is to simply check if a matching discovery controller is
available.

But if we do that blindly, we may erroneously use a temporary
connection that has been set up by some foreign process, and may go
away under us. That's why we do the kato check. But as you pointed out
yourself in your comment on 01/10, that would completely disable using
existing controllers on older kernels that don't have the "kato"
attribute. This patch changes the behavior such that if no kato-
attribute is found, we trust the user if she specified the controller
explicitly, but we don't trust just random controllers found in sysfs.

Thanks,
Martin




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

[PATCH 03/10] 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 dcee0c4..223bd04 100644
--- a/fabrics.c
+++ b/fabrics.c
@@ -1378,7 +1378,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, true))
+	if (cfg.device && !ctrl_matches_connectargs(cfg.device, cargs, true)) {
+		free(cfg.device);
+		cfg.device = NULL;
+	}
+	if (!cfg.device)
 		cfg.device = find_ctrl_with_connectargs(cargs);
 	free_connect_args(cargs);
 
@@ -1562,8 +1566,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.29.2


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

[PATCH 04/10] nvme-connect-all(1): fix documentation for --quiet/-S

[mwilck]

From: Martin Wilck <mwilck@suse.com>

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.29.2


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

Re: [PATCH 04/10] nvme-connect-all(1): fix documentation for --quiet/-S

[Chaitanya Kulkarni]

On 3/5/21 16:36, mwilck@suse.com wrote:
> From: Martin Wilck <mwilck@suse.com>
>
> Signed-off-by: Martin Wilck <mwilck@suse.com>

Stale date reference 01/20/2021 to current date, otherwise
Looks good.

Reviewed-by: Chaitanya Kulkarni <chaitanya.kulkarni@wdc.com>


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

Re: [PATCH 04/10] nvme-connect-all(1): fix documentation for --quiet/-S

[Sagi Grimberg]

Reviewed-by: Sagi Grimberg <sagi@grimberg.me>

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

[PATCH 05/10] 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.

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.29.2


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

Re: [PATCH 05/10] nvme: add some simplifying macros for __attribute__((cleanup()))

[Chaitanya Kulkarni]

On 3/5/21 16:36, mwilck@suse.com wrote:
> 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.
>
> Signed-off-by: Martin Wilck <mwilck@suse.com>
Looks good.

Reviewed-by: Chaitanya Kulkarni <chaitanya.kulkarni@wdc.com>


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

Re: [PATCH 05/10] nvme: add some simplifying macros for __attribute__((cleanup()))

[Sagi Grimberg]

Reviewed-by: Sagi Grimberg <sagi@grimberg.me>

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

[PATCH 06/10] 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.

Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 Makefile   |  2 +-
 fabrics.c  | 85 ++++++++++++++++++++++++++++-----------------------
 util/log.c | 90 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
 util/log.h | 34 +++++++++++++++++++++
 4 files changed, 171 insertions(+), 40 deletions(-)
 create mode 100644 util/log.c
 create mode 100644 util/log.h

diff --git a/Makefile b/Makefile
index 3412e5d..1fe693c 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/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 223bd04..04f0aef 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 = {
@@ -381,7 +383,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;
 	}
 
@@ -389,7 +391,7 @@ static char *find_ctrl_with_connectargs(struct connect_args *args)
 		if (ctrl_matches_connectargs(devices[i]->d_name, args, false)) {
 			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;
 		}
@@ -451,7 +453,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;
@@ -459,8 +461,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;
@@ -468,7 +470,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;
@@ -495,7 +497,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);
@@ -510,7 +512,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;
 	}
@@ -564,7 +566,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;
 	}
@@ -789,7 +791,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;
 	}
@@ -798,7 +800,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);
@@ -945,13 +947,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;
 		}
 	}
@@ -1045,7 +1047,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;
 	}
 
@@ -1061,14 +1063,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;
 	}
@@ -1096,7 +1098,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;
 	}
@@ -1185,7 +1187,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;
 	}
@@ -1231,7 +1233,7 @@ retry:
 			p += len;
 			break;
 		default:
-			fprintf(stderr, "skipping unsupported adrfam\n");
+			msg(LOG_ERR, "skipping unsupported adrfam\n");
 			return -EINVAL;
 		}
 		break;
@@ -1246,7 +1248,7 @@ retry:
 			p += len;
 			break;
 		default:
-			fprintf(stderr, "skipping unsupported adrfam\n");
+			msg(LOG_ERR, "skipping unsupported adrfam\n");
 			return -EINVAL;
 		}
 		break;
@@ -1336,9 +1338,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);
@@ -1400,7 +1400,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)
@@ -1419,12 +1419,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:
@@ -1436,12 +1436,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;
 	}
 
@@ -1457,7 +1457,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;
 	}
@@ -1470,14 +1470,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;
@@ -1531,6 +1531,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"),
@@ -1552,7 +1553,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()
@@ -1574,6 +1575,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) {
@@ -1645,7 +1652,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;
 	}
@@ -1683,7 +1690,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;
 	}
@@ -1757,7 +1764,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;
 	}
@@ -1765,7 +1772,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);
@@ -1776,7 +1783,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);
 	}
@@ -1800,7 +1807,7 @@ int fabrics_disconnect_all(const char *desc, int argc, char **argv)
 
 	err = scan_subsystems(&t, NULL, 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.29.2


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

Re: [PATCH 06/10] nvme-cli: add generic logging functionality

[Chaitanya Kulkarni]

On 3/5/21 16:36, mwilck@suse.com wrote:
>  
> -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
Line is too long, can you split ?

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

Re: [PATCH 06/10] nvme-cli: add generic logging functionality

[Chaitanya Kulkarni]

On 3/5/21 16:36, mwilck@suse.com wrote:
> 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.
>
> Signed-off-by: Martin Wilck <mwilck@suse.com>
Looks good.

Reviewed-by: Chaitanya Kulkarni <chaitanya.kulkarni@wdc.com>

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

Re: [PATCH 06/10] nvme-cli: add generic logging functionality

[Sagi Grimberg]

> 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.
> 
> Signed-off-by: Martin Wilck <mwilck@suse.com>
> ---
>   Makefile   |  2 +-
>   fabrics.c  | 85 ++++++++++++++++++++++++++++-----------------------
>   util/log.c | 90 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
>   util/log.h | 34 +++++++++++++++++++++

I think it makes sense to do a wider sweep also for nvme.c and friends
no?

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

Re: [PATCH 06/10] nvme-cli: add generic logging functionality

[Martin Wilck]

On Mon, 2021-03-15 at 10:45 -0700, Sagi Grimberg wrote:
> 
> > 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.
> > 
> > Signed-off-by: Martin Wilck <mwilck@suse.com>
> > ---
> >   Makefile   |  2 +-
> >   fabrics.c  | 85 ++++++++++++++++++++++++++++---------------------
> > --
> >   util/log.c | 90
> > ++++++++++++++++++++++++++++++++++++++++++++++++++++++
> >   util/log.h | 34 +++++++++++++++++++++
> 
> I think it makes sense to do a wider sweep also for nvme.c and
> friends
> no?

Yes, but could we do that wider sweep separately, or do you consider it
a requirement?

Regards,
Martin







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

[PATCH 07/10] 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.

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 04f0aef..8a1586f 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,
 				     bool assume_persistent)
 {
 	struct connect_args cargs;
@@ -927,7 +932,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;
 
@@ -1677,7 +1682,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] = {};
@@ -1715,7 +1720,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;
@@ -1737,7 +1742,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 c0e2d51..fc581f8 100644
--- a/nvme-topology.c
+++ b/nvme-topology.c
@@ -45,7 +45,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 2653939..9064e83 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 3fb1060..5b4c1e1 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, 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.29.2


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

Re: [PATCH 07/10] nvme: convert some function arguments from "char *" to "const char *"

[Chaitanya Kulkarni]

On 3/5/21 16:36, mwilck@suse.com wrote:
> 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.
>
> Signed-off-by: Martin Wilck <mwilck@suse.com>
Looks good.

Reviewed-by: Chaitanya Kulkarni <chaitanya.kulkarni@wdc.com>


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

Re: [PATCH 07/10] nvme: convert some function arguments from "char *" to "const char *"

[Sagi Grimberg]

Reviewed-by: Sagi Grimberg <sagi@grimberg.me>

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

[PATCH 08/10] 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.

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 8a1586f..c965beb 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.29.2


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

Re: [PATCH 08/10] fabrics: use "const char *" in struct config

[Chaitanya Kulkarni]

On 3/5/21 16:36, mwilck@suse.com wrote:
> 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.
>
> Signed-off-by: Martin Wilck <mwilck@suse.com>
Looks good.

Reviewed-by: Chaitanya Kulkarni <chaitanya.kulkarni@wdc.com>


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

Re: [PATCH 08/10] fabrics: use "const char *" in struct config

[Sagi Grimberg]

Reviewed-by: Sagi Grimberg <sagi@grimberg.me>

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

[PATCH 09/10] 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.

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 c965beb..f2ff511 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>
@@ -309,7 +310,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;
 
@@ -369,6 +370,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;
 }
@@ -425,13 +428,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);
 }
 
@@ -1285,7 +1293,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);
@@ -1371,9 +1379,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;
@@ -1457,7 +1467,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");
@@ -1479,6 +1489,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) {
@@ -1522,7 +1533,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.29.2


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

Re: [PATCH 09/10] fabrics: fix some memory leaks

[Chaitanya Kulkarni]

On 3/5/21 16:36, mwilck@suse.com wrote:
> 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.
>
> Signed-off-by: Martin Wilck <mwilck@suse.com>
Looks good.

Reviewed-by: Chaitanya Kulkarni <chaitanya.kulkarni@wdc.com>

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

Re: [PATCH 09/10] fabrics: fix some memory leaks

[Sagi Grimberg]

Reviewed-by: Sagi Grimberg <sagi@grimberg.me>

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

[PATCH 10/10] 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 f2ff511..1fcc6cf 100644
--- a/fabrics.c
+++ b/fabrics.c
@@ -1508,6 +1508,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;
@@ -1535,6 +1538,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.29.2


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