[ndctl PATCH 0/4] misc fixups for ndctl-monitor

[ndctl PATCH 0/4] misc fixups for ndctl-monitor

[Vishal Verma]

Fix a couple of issues reported by static analysis, improve the error
reporting, and add a missing option to the monitor man page

Vishal Verma (4):
  ndctl, monitor: Add a config-file section to the man page
  ndctl, monitor: fix memory leak in read_config_file
  ndctl, monitor: Fix memory leak in monitor_event
  ndctl, monitor: improve error reporting throughout monitor.c

 Documentation/ndctl/ndctl-monitor.txt |   5 ++
 ndctl/monitor.c                       | 129 +++++++++++++++++-----------------
 2 files changed, 69 insertions(+), 65 deletions(-)

-- 
2.14.4

_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

[ndctl PATCH 1/4] ndctl, monitor: Add a config-file section to the man page

[Vishal Verma]

The ndctl monitor man page was missing documentation for the
--config-file option. Add it, explaining that the option overrides the
system default config file.

Fixes: 02ce9c6bab41 ("ndctl, documentation: add man page for monitor")
Cc: QI Fuli <qi.fuli@jp.fujitsu.com>
Signed-off-by: Vishal Verma <vishal.l.verma@intel.com>
---
 Documentation/ndctl/ndctl-monitor.txt | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/Documentation/ndctl/ndctl-monitor.txt b/Documentation/ndctl/ndctl-monitor.txt
index 872cd6e..762073e 100644
--- a/Documentation/ndctl/ndctl-monitor.txt
+++ b/Documentation/ndctl/ndctl-monitor.txt
@@ -69,6 +69,11 @@ OPTIONS
 --log=<file | syslog | standard>::
 	Output notifications to <file>, syslog or standard output.
 
+-c::
+--config-file=::
+	Provide the config file to use. This overrides the default config
+	typically found in /etc/ndctl/
+
 --daemon::
 	Run a monitor as a daemon.
 
-- 
2.14.4

_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

[ndctl PATCH 2/4] ndctl, monitor: fix memory leak in read_config_file

[Vishal Verma]

Static analysis reports that we leak 'buf' in the above function. Free
that, and refactor some of the error reporting to clean up the flow.
Since the parser loop was modifying 'buf', it wasn't possible to
directly free it. Introduce a 'seek' variable to seek within the buf so
that buf itself remains untouched, and can later be freed.

Fixes: 80dd56d82dd3 ("ndctl, monitor: add main ndctl monitor configuration file")
Cc: QI Fuli <qi.fuli@jp.fujitsu.com>
Signed-off-by: Vishal Verma <vishal.l.verma@intel.com>
---
 ndctl/monitor.c | 44 +++++++++++++++++++++++---------------------
 1 file changed, 23 insertions(+), 21 deletions(-)

diff --git a/ndctl/monitor.c b/ndctl/monitor.c
index abab45f..4d25c2b 100644
--- a/ndctl/monitor.c
+++ b/ndctl/monitor.c
@@ -478,9 +478,9 @@ static int read_config_file(struct ndctl_ctx *ctx, struct monitor *_monitor,
 		struct util_filter_params *_param)
 {
 	FILE *f;
-	int line = 0;
 	size_t len = 0;
-	char *buf, *value, *config_file;
+	int line = 0, rc = 0;
+	char *buf = NULL, *seek, *value, *config_file;
 
 	if (_monitor->config_file)
 		config_file = strdup(_monitor->config_file);
@@ -488,32 +488,36 @@ static int read_config_file(struct ndctl_ctx *ctx, struct monitor *_monitor,
 		config_file = strdup(DEF_CONF_FILE);
 	if (!config_file) {
 		fail("strdup default config file failed\n");
+		rc = -ENOMEM;
 		goto out;
 	}
 
 	buf = malloc(BUF_SIZE);
 	if (!buf) {
 		fail("malloc read config-file buf error\n");
+		rc = -ENOMEM;
 		goto out;
 	}
+	seek = buf;
 
 	f = fopen(config_file, "r");
 	if (!f) {
 		fail("config-file: %s cannot be opened\n", config_file);
+		rc = -errno;
 		goto out;
 	}
 
-	while (fgets(buf, BUF_SIZE, f)) {
+	while (fgets(seek, BUF_SIZE, f)) {
 		value = NULL;
 		line++;
 
-		while (isspace(*buf))
-			buf++;
+		while (isspace(*seek))
+			seek++;
 
-		if (*buf == '#' || *buf == '\0')
+		if (*seek == '#' || *seek == '\0')
 			continue;
 
-		value = strchr(buf, '=');
+		value = strchr(seek, '=');
 		if (!value) {
 			fail("config-file syntax error, skip line[%i]\n", line);
 			continue;
@@ -525,12 +529,12 @@ static int read_config_file(struct ndctl_ctx *ctx, struct monitor *_monitor,
 		while (isspace(value[0]))
 			value++;
 
-		len = strlen(buf);
+		len = strlen(seek);
 		if (len == 0)
 			continue;
-		while (isspace(buf[len-1]))
+		while (isspace(seek[len-1]))
 			len--;
-		buf[len] = '\0';
+		seek[len] = '\0';
 
 		len = strlen(value);
 		if (len == 0)
@@ -542,22 +546,20 @@ static int read_config_file(struct ndctl_ctx *ctx, struct monitor *_monitor,
 		if (len == 0)
 			continue;
 
-		parse_config(&_param->bus, "bus", value, buf);
-		parse_config(&_param->dimm, "dimm", value, buf);
-		parse_config(&_param->region, "region", value, buf);
-		parse_config(&_param->namespace, "namespace", value, buf);
-		parse_config(&_monitor->dimm_event, "dimm-event", value, buf);
+		parse_config(&_param->bus, "bus", value, seek);
+		parse_config(&_param->dimm, "dimm", value, seek);
+		parse_config(&_param->region, "region", value, seek);
+		parse_config(&_param->namespace, "namespace", value, seek);
+		parse_config(&_monitor->dimm_event, "dimm-event", value, seek);
 
 		if (!_monitor->log)
-			parse_config(&_monitor->log, "log", value, buf);
+			parse_config(&_monitor->log, "log", value, seek);
 	}
 	fclose(f);
-	free(config_file);
-	return 0;
 out:
-	if (config_file)
-		free(config_file);
-	return 1;
+	free(buf);
+	free(config_file);
+	return rc;
 }
 
 int cmd_monitor(int argc, const char **argv, void *ctx)
-- 
2.14.4

_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

[ndctl PATCH 3/4] ndctl, monitor: Fix memory leak in monitor_event

[Vishal Verma]

Static analysis reports we were leaking the 'events' buffer in the above
function. We were also returning a simple '1' for every error case. Take
this chance to return a more meaningful 'errno' in each case, while also
fixing up the resource leak.

Fixes: fdf6b6844ccf ("ndctl, monitor: add a new command - monitor")
Cc: QI Fuli <qi.fuli@jp.fujitsu.com>
Signed-off-by: Vishal Verma <vishal.l.verma@intel.com>
---
 ndctl/monitor.c | 27 ++++++++++++++++++---------
 1 file changed, 18 insertions(+), 9 deletions(-)

diff --git a/ndctl/monitor.c b/ndctl/monitor.c
index 4d25c2b..7fc2aaa 100644
--- a/ndctl/monitor.c
+++ b/ndctl/monitor.c
@@ -360,32 +360,35 @@ static int monitor_event(struct ndctl_ctx *ctx,
 		struct monitor_filter_arg *mfa)
 {
 	struct epoll_event ev, *events;
-	int nfds, epollfd, i, rc;
+	int nfds, epollfd, i, rc = 0;
 	struct monitor_dimm *mdimm;
 	char buf;
 
 	events = calloc(mfa->num_dimm, sizeof(struct epoll_event));
 	if (!events) {
 		err(ctx, "malloc for events error\n");
-		return 1;
+		return -ENOMEM;
 	}
 	epollfd = epoll_create1(0);
 	if (epollfd == -1) {
 		err(ctx, "epoll_create1 error\n");
-		return 1;
+		rc = -errno;
+		goto out;
 	}
 	list_for_each(&mfa->dimms, mdimm, list) {
 		memset(&ev, 0, sizeof(ev));
 		rc = pread(mdimm->health_eventfd, &buf, sizeof(buf), 0);
 		if (rc < 0) {
 			err(ctx, "pread error\n");
-			return 1;
+			rc = -errno;
+			goto out;
 		}
 		ev.data.ptr = mdimm;
 		if (epoll_ctl(epollfd, EPOLL_CTL_ADD,
 				mdimm->health_eventfd, &ev) != 0) {
 			err(ctx, "epoll_ctl error\n");
-			return 1;
+			rc = -errno;
+			goto out;
 		}
 	}
 
@@ -394,7 +397,8 @@ static int monitor_event(struct ndctl_ctx *ctx,
 		nfds = epoll_wait(epollfd, events, mfa->num_dimm, -1);
 		if (nfds <= 0) {
 			err(ctx, "epoll_wait error\n");
-			return 1;
+			rc = -errno;
+			goto out;
 		}
 		for (i = 0; i < nfds; i++) {
 			mdimm = events[i].data.ptr;
@@ -404,13 +408,18 @@ static int monitor_event(struct ndctl_ctx *ctx,
 						ndctl_dimm_get_devname(mdimm->dimm));
 			}
 			rc = pread(mdimm->health_eventfd, &buf, sizeof(buf), 0);
-			if (rc < 0)
-				fail("pread error\n");
+			if (rc < 0) {
+				err(ctx, "pread error\n");
+				rc = -errno;
+				goto out;
+			}
 		}
 		if (did_fail)
 			return 1;
 	}
-	return 0;
+ out:
+	free(events);
+	return rc;
 }
 
 static int parse_monitor_event(struct monitor *_monitor, struct ndctl_ctx *ctx)
-- 
2.14.4

_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

[ndctl PATCH 4/4] ndctl, monitor: improve error reporting throughout monitor.c

[Vishal Verma]

In several places in the ndctl monitor, we were losing useful error
information (from 'errno' for example), and just returning a simple '1'
or '-1'. Fix these to capture and propagate the correct errors
everywhere.

In the case of notify_dimm_event(), don't error out for failures of
json_object_new_*. Follow the precedent of util/json.c and only add the
object to its parent if the 'new' function was successful.

Cc: QI Fuli <qi.fuli@jp.fujitsu.com>
Signed-off-by: Vishal Verma <vishal.l.verma@intel.com>
---
 ndctl/monitor.c | 58 +++++++++++++++++++++++----------------------------------
 1 file changed, 23 insertions(+), 35 deletions(-)

diff --git a/ndctl/monitor.c b/ndctl/monitor.c
index 7fc2aaa..dbad7aa 100644
--- a/ndctl/monitor.c
+++ b/ndctl/monitor.c
@@ -174,45 +174,30 @@ static int notify_dimm_event(struct monitor_dimm *mdimm)
 	jmsg = json_object_new_object();
 	if (!jmsg) {
 		fail("\n");
-		return -1;
+		return -ENOMEM;
 	}
 
 	clock_gettime(CLOCK_REALTIME, &ts);
 	sprintf(timestamp, "%10ld.%09ld", ts.tv_sec, ts.tv_nsec);
 	jobj = json_object_new_string(timestamp);
-	if (!jobj) {
-		fail("\n");
-		return -1;
-	}
-	json_object_object_add(jmsg, "timestamp", jobj);
+	if (jobj)
+		json_object_object_add(jmsg, "timestamp", jobj);
 
 	jobj = json_object_new_int(getpid());
-	if (!jobj) {
-		fail("\n");
-		return -1;
-	}
-	json_object_object_add(jmsg, "pid", jobj);
+	if (jobj)
+		json_object_object_add(jmsg, "pid", jobj);
 
 	jobj = dimm_event_to_json(mdimm);
-	if (!jobj) {
-		fail("\n");
-		return -1;
-	}
-	json_object_object_add(jmsg, "event", jobj);
+	if (jobj)
+		json_object_object_add(jmsg, "event", jobj);
 
 	jdimm = util_dimm_to_json(mdimm->dimm, 0);
-	if (!jdimm) {
-		fail("\n");
-		return -1;
-	}
-	json_object_object_add(jmsg, "dimm", jdimm);
+	if (jdimm)
+		json_object_object_add(jmsg, "dimm", jdimm);
 
 	jobj = util_dimm_health_to_json(mdimm->dimm);
-	if (!jobj) {
-		fail("\n");
-		return -1;
-	}
-	json_object_object_add(jdimm, "health", jobj);
+	if (jobj)
+		json_object_object_add(jdimm, "health", jobj);
 
 	if (monitor.human)
 		notice(ctx, "%s\n", json_object_to_json_string_ext(jmsg,
@@ -403,9 +388,12 @@ static int monitor_event(struct ndctl_ctx *ctx,
 		for (i = 0; i < nfds; i++) {
 			mdimm = events[i].data.ptr;
 			if (util_dimm_event_filter(mdimm, monitor.event_flags)) {
-				if (notify_dimm_event(mdimm))
+				rc = notify_dimm_event(mdimm);
+				if (rc) {
 					fail("%s: notify dimm event failed\n",
 						ndctl_dimm_get_devname(mdimm->dimm));
+					goto out;
+				}
 			}
 			rc = pread(mdimm->health_eventfd, &buf, sizeof(buf), 0);
 			if (rc < 0) {
@@ -601,7 +589,7 @@ int cmd_monitor(int argc, const char **argv, void *ctx)
 	const char *prefix = "./";
 	struct util_filter_ctx fctx = { 0 };
 	struct monitor_filter_arg mfa = { 0 };
-	int i;
+	int i, rc;
 
 	argc = parse_options_prefix(argc, argv, prefix, options, u, 0);
 	for (i = 0; i < argc; i++) {
@@ -614,7 +602,8 @@ int cmd_monitor(int argc, const char **argv, void *ctx)
 	ndctl_set_log_fn((struct ndctl_ctx *)ctx, log_standard);
 	ndctl_set_log_priority((struct ndctl_ctx *)ctx, LOG_NOTICE);
 
-	if (read_config_file((struct ndctl_ctx *)ctx, &monitor, &param))
+	rc = read_config_file((struct ndctl_ctx *)ctx, &monitor, &param);
+	if (rc)
 		goto out;
 
 	if (monitor.log) {
@@ -648,18 +637,17 @@ int cmd_monitor(int argc, const char **argv, void *ctx)
 	mfa.maxfd_dimm = -1;
 	mfa.flags = 0;
 
-	if (util_filter_walk(ctx, &fctx, &param))
+	rc = util_filter_walk(ctx, &fctx, &param);
+	if (rc)
 		goto out;
 
 	if (!mfa.num_dimm) {
 		err((struct ndctl_ctx *)ctx, "no dimms to monitor\n");
+		rc = -ENXIO;
 		goto out;
 	}
 
-	if (monitor_event(ctx, &mfa))
-		goto out;
-
-	return 0;
+	rc = monitor_event(ctx, &mfa);
 out:
-	return 1;
+	return rc;
 }
-- 
2.14.4

_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

RE: [ndctl PATCH 0/4] misc fixups for ndctl-monitor

[Qi, Fuli]

> -----Original Message-----
> From: Vishal Verma [mailto:vishal.l.verma@intel.com]
> Sent: Friday, July 20, 2018 8:00 AM
> To: linux-nvdimm@lists.01.org
> Cc: Qi, Fuli/斉 福利 <qi.fuli@jp.fujitsu.com>; Vishal Verma
> <vishal.l.verma@intel.com>
> Subject: [ndctl PATCH 0/4] misc fixups for ndctl-monitor
> 
> Fix a couple of issues reported by static analysis, improve the error reporting,
> and add a missing option to the monitor man page
> 
> Vishal Verma (4):
>   ndctl, monitor: Add a config-file section to the man page
>   ndctl, monitor: fix memory leak in read_config_file
>   ndctl, monitor: Fix memory leak in monitor_event
>   ndctl, monitor: improve error reporting throughout monitor.c
> 

Hi Vishal,

Looks good to me.
Thank you very much.
Qi

>  Documentation/ndctl/ndctl-monitor.txt |   5 ++
>  ndctl/monitor.c                       | 129 +++++++++++++++++-----------------
>  2 files changed, 69 insertions(+), 65 deletions(-)
> 
> --
> 2.14.4
> 
> 


_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

Re: [ndctl PATCH 0/4] misc fixups for ndctl-monitor

[Vishal Verma]

On Thu, 2018-07-19 at 23:59 +0000, Qi, Fuli wrote:
> > -----Original Message-----
> > From: Vishal Verma [mailto:vishal.l.verma@intel.com]
> > Sent: Friday, July 20, 2018 8:00 AM
> > To: linux-nvdimm@lists.01.org
> > Cc: Qi, Fuli/斉 福利 <qi.fuli@jp.fujitsu.com>; Vishal Verma
> > <vishal.l.verma@intel.com>
> > Subject: [ndctl PATCH 0/4] misc fixups for ndctl-monitor
> > 
> > Fix a couple of issues reported by static analysis, improve the error
> > reporting,
> > and add a missing option to the monitor man page
> > 
> > Vishal Verma (4):
> >   ndctl, monitor: Add a config-file section to the man page
> >   ndctl, monitor: fix memory leak in read_config_file
> >   ndctl, monitor: Fix memory leak in monitor_event
> >   ndctl, monitor: improve error reporting throughout monitor.c
> > 
> 
> Hi Vishal,
> 
> Looks good to me.
> Thank you very much.

Hi Qi,

Can I add your Reviewed-by tags for the series then?

Thanks,
	-Vishal

_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

RE: [ndctl PATCH 0/4] misc fixups for ndctl-monitor

[Qi, Fuli]

> -----Original Message-----
> From: Vishal Verma [mailto:vishal@kernel.org]
> Sent: Friday, July 20, 2018 12:26 PM
> To: Qi, Fuli/斉 福利 <qi.fuli@jp.fujitsu.com>; 'Vishal Verma'
> <vishal.l.verma@intel.com>; linux-nvdimm@lists.01.org
> Subject: Re: [ndctl PATCH 0/4] misc fixups for ndctl-monitor
> 
> On Thu, 2018-07-19 at 23:59 +0000, Qi, Fuli wrote:
> > > -----Original Message-----
> > > From: Vishal Verma [mailto:vishal.l.verma@intel.com]
> > > Sent: Friday, July 20, 2018 8:00 AM
> > > To: linux-nvdimm@lists.01.org
> > > Cc: Qi, Fuli/斉 福利 <qi.fuli@jp.fujitsu.com>; Vishal Verma
> > > <vishal.l.verma@intel.com>
> > > Subject: [ndctl PATCH 0/4] misc fixups for ndctl-monitor
> > >
> > > Fix a couple of issues reported by static analysis, improve the
> > > error reporting, and add a missing option to the monitor man page
> > >
> > > Vishal Verma (4):
> > >   ndctl, monitor: Add a config-file section to the man page
> > >   ndctl, monitor: fix memory leak in read_config_file
> > >   ndctl, monitor: Fix memory leak in monitor_event
> > >   ndctl, monitor: improve error reporting throughout monitor.c
> > >
> >
> > Hi Vishal,
> >
> > Looks good to me.
> > Thank you very much.
> 
> Hi Qi,
> 
> Can I add your Reviewed-by tags for the series then?
> 
> Thanks,
> 	-Vishal
> 

Yes, please.

Thanks,
Qi

_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm