[ndctl PATCH v4 0/4] cxl/monitor and ndctl/monitor fixes

[ndctl PATCH v4 0/4] cxl/monitor and ndctl/monitor fixes

[Li Zhijian]

V4:
  - Add reviewed tags and minor fixes including comment style and
changelog update
  - combine "cxl/monitor: use strcmp to compare the reserved word" and
    "ndctl/monitor: use strcmp to compare the reserved word" to one
    patch
  - Drop "cxl/monitor: always log started message" which would break a
    json output.

V3:
- update comit log of patch3 and patch6 per Dave's comments.

V2:
- exchange order of previous patch1 and patch2
- add reviewed tag in patch5
- commit log improvements

It mainly fix monitor not working when log file is specified. For
example
$ cxl monitor -l ./cxl-monitor.log
It seems that someone missed something at the begining.

Furture, it compares the filename with reserved word more accurately

patch1-2: It re-enables logfile(including default_log) functionality
and simplify the sanity check in the combination relative path file
and daemon mode.

patch3 and patch6 change strncmp to strcmp to compare the acurrate
reserved words.


*** BLURB HERE ***

Li Zhijian (4):
  cxl/monitor: Enable default_log and refactor sanity check
  cxl/monitor: replace monitor.log_file with monitor.ctx.log_file
  ndctl: use strcmp for reserved word in monitor commands
  Documentation/cxl/cxl-monitor.txt: Fix inaccurate description

 Documentation/cxl/cxl-monitor.txt |  3 +--
 cxl/monitor.c                     | 43 ++++++++++++++++---------------
 ndctl/monitor.c                   |  4 +--
 3 files changed, 25 insertions(+), 25 deletions(-)

-- 
2.29.2

[ndctl PATCH v4 1/4] cxl/monitor: Enable default_log and refactor sanity check

[Li Zhijian]

The default_log(/var/log/cxl-monitor.log) should be used when no '-l'
argument is specified in daemon mode, but it was not working at all.

Simplify the sanity checks so that the default log file is assigned
correctly, and the behavior is consistent with the documentation.

Remove the unnecessary fix_filename() for monitor.log since
parse_options_prefix() has done similar stuff if needed.

Reviewed-by: Dave Jiang <dave.jiang@intel.com>
Signed-off-by: Li Zhijian <lizhijian@fujitsu.com>
---
V4: add reviewed tag and minor fixes: comment style and change log.
V2: exchange order of previous patch1 and patch2 # Alison
    a few commit log updated
Signed-off-by: Li Zhijian <lizhijian@fujitsu.com>
---
 cxl/monitor.c | 38 ++++++++++++++++++++------------------
 1 file changed, 20 insertions(+), 18 deletions(-)

diff --git a/cxl/monitor.c b/cxl/monitor.c
index e3469b9a4792..d8245ed8d0e9 100644
--- a/cxl/monitor.c
+++ b/cxl/monitor.c
@@ -164,6 +164,7 @@ int cmd_monitor(int argc, const char **argv, struct cxl_ctx *ctx)
 	};
 	const char *prefix ="./";
 	int rc = 0, i;
+	const char *log;
 
 	argc = parse_options_prefix(argc, argv, prefix, options, u, 0);
 	for (i = 0; i < argc; i++)
@@ -171,32 +172,33 @@ int cmd_monitor(int argc, const char **argv, struct cxl_ctx *ctx)
 	if (argc)
 		usage_with_options(u, options);
 
+	/* sanity check */
+	if (monitor.daemon && monitor.log && !strncmp(monitor.log, "./", 2)) {
+		error("relative path or 'standard' are not compatible with daemon mode\n");
+		return -EINVAL;
+	}
+
 	log_init(&monitor.ctx, "cxl/monitor", "CXL_MONITOR_LOG");
-	monitor.ctx.log_fn = log_standard;
+	if (monitor.log)
+		log = monitor.log;
+	else
+		log = monitor.daemon ? default_log : "./standard";
 
 	if (monitor.verbose)
 		monitor.ctx.log_priority = LOG_DEBUG;
 	else
 		monitor.ctx.log_priority = LOG_INFO;
 
-	if (monitor.log) {
-		if (strncmp(monitor.log, "./", 2) != 0)
-			fix_filename(prefix, (const char **)&monitor.log);
-		if (strncmp(monitor.log, "./standard", 10) == 0 && !monitor.daemon) {
-			monitor.ctx.log_fn = log_standard;
-		} else {
-			const char *log = monitor.log;
-
-			if (!monitor.log)
-				log = default_log;
-			monitor.log_file = fopen(log, "a+");
-			if (!monitor.log_file) {
-				rc = -errno;
-				error("open %s failed: %d\n", monitor.log, rc);
-				goto out;
-			}
-			monitor.ctx.log_fn = log_file;
+	if (strncmp(log, "./standard", 10) == 0)
+		monitor.ctx.log_fn = log_standard;
+	else {
+		monitor.log_file = fopen(log, "a+");
+		if (!monitor.log_file) {
+			rc = -errno;
+			error("open %s failed: %d\n", log, rc);
+			goto out;
 		}
+		monitor.ctx.log_fn = log_file;
 	}
 
 	if (monitor.daemon) {
-- 
2.29.2

[ndctl PATCH v4 2/4] cxl/monitor: replace monitor.log_file with monitor.ctx.log_file

[Li Zhijian]

Commit ba5825b0b7e0 ("ndctl/monitor: move common logging functions to util/log.c")
have replaced monitor.log_file with monitor.ctx.log_file for
ndctl-monitor, but for cxl-monitor, it forgot to do such work.

So where user specifies its own logfile, a segmentation fault will be
trggered like below:

 # build/cxl/cxl monitor -l ./monitor.log
Segmentation fault (core dumped)

Fixes: 299f69f974a6 ("cxl/monitor: add a new monitor command for CXL trace events")
Reviewed-by: Dave Jiang <dave.jiang@intel.com>
Signed-off-by: Li Zhijian <lizhijian@fujitsu.com>
---
V4: add reviewed tag
V2: exchange order of previous patch1 and patch2 # Alison
    a few commit log updated
---
 cxl/monitor.c | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/cxl/monitor.c b/cxl/monitor.c
index d8245ed8d0e9..e83455b63d35 100644
--- a/cxl/monitor.c
+++ b/cxl/monitor.c
@@ -37,7 +37,6 @@ const char *default_log = "/var/log/cxl-monitor.log";
 static struct monitor {
 	const char *log;
 	struct log_ctx ctx;
-	FILE *log_file;
 	bool human;
 	bool verbose;
 	bool daemon;
@@ -192,8 +191,8 @@ int cmd_monitor(int argc, const char **argv, struct cxl_ctx *ctx)
 	if (strncmp(log, "./standard", 10) == 0)
 		monitor.ctx.log_fn = log_standard;
 	else {
-		monitor.log_file = fopen(log, "a+");
-		if (!monitor.log_file) {
+		monitor.ctx.log_file = fopen(log, "a+");
+		if (!monitor.ctx.log_file) {
 			rc = -errno;
 			error("open %s failed: %d\n", log, rc);
 			goto out;
@@ -212,7 +211,7 @@ int cmd_monitor(int argc, const char **argv, struct cxl_ctx *ctx)
 	rc = monitor_event(ctx);
 
 out:
-	if (monitor.log_file)
-		fclose(monitor.log_file);
+	if (monitor.ctx.log_file)
+		fclose(monitor.ctx.log_file);
 	return rc;
 }
-- 
2.29.2

[ndctl PATCH v4 3/4] ndctl: use strcmp for reserved word in monitor commands

[Li Zhijian]

According to the tool's documentation, when '-l standard' is specified,
log would be output to the stdout. But since it's using strncmp(a, b, 10)
to compare the former 10 characters, it will also wrongly detect a filename
starting with a substring 'standard' as stdout.

For example:
$ cxl monitor -l standard.log

User is most likely want to save log to ./standard.log instead of stdout.

Reviewed-by: Dave Jiang <dave.jiang@intel.com>
Signed-off-by: Li Zhijian <lizhijian@fujitsu.com>
---
V4: combine ndctl/monitor to one patch
V3: Improve commit log # Dave
V2: commit log updated # Dave
Signed-off-by: Li Zhijian <lizhijian@fujitsu.com>
---
 cxl/monitor.c   | 2 +-
 ndctl/monitor.c | 4 ++--
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/cxl/monitor.c b/cxl/monitor.c
index e83455b63d35..a85452a4dc82 100644
--- a/cxl/monitor.c
+++ b/cxl/monitor.c
@@ -188,7 +188,7 @@ int cmd_monitor(int argc, const char **argv, struct cxl_ctx *ctx)
 	else
 		monitor.ctx.log_priority = LOG_INFO;
 
-	if (strncmp(log, "./standard", 10) == 0)
+	if (strcmp(log, "./standard") == 0)
 		monitor.ctx.log_fn = log_standard;
 	else {
 		monitor.ctx.log_file = fopen(log, "a+");
diff --git a/ndctl/monitor.c b/ndctl/monitor.c
index 89903def63d4..bd8a74863476 100644
--- a/ndctl/monitor.c
+++ b/ndctl/monitor.c
@@ -610,9 +610,9 @@ int cmd_monitor(int argc, const char **argv, struct ndctl_ctx *ctx)
 	if (monitor.log) {
 		if (strncmp(monitor.log, "./", 2) != 0)
 			fix_filename(prefix, (const char **)&monitor.log);
-		if (strncmp(monitor.log, "./syslog", 8) == 0)
+		if (strcmp(monitor.log, "./syslog") == 0)
 			monitor.ctx.log_fn = log_syslog;
-		else if (strncmp(monitor.log, "./standard", 10) == 0)
+		else if (strcmp(monitor.log, "./standard") == 0)
 			monitor.ctx.log_fn = log_standard;
 		else {
 			monitor.ctx.log_file = fopen(monitor.log, "a+");
-- 
2.29.2

[ndctl PATCH v4 4/4] Documentation/cxl/cxl-monitor.txt: Fix inaccurate description

[Li Zhijian]

No syslog is supported by cxl-monitor

Reviewed-by: Dave Jiang <dave.jiang@intel.com>
Signed-off-by: Li Zhijian <lizhijian@fujitsu.com>
---
V2: add Reviewed-by tag
---
 Documentation/cxl/cxl-monitor.txt | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/Documentation/cxl/cxl-monitor.txt b/Documentation/cxl/cxl-monitor.txt
index 3fc992e4d4d9..c284099f16c3 100644
--- a/Documentation/cxl/cxl-monitor.txt
+++ b/Documentation/cxl/cxl-monitor.txt
@@ -39,8 +39,7 @@ OPTIONS
 --log=::
 	Send log messages to the specified destination.
 	- "<file>":
-	  Send log messages to specified <file>. When fopen() is not able
-	  to open <file>, log messages will be forwarded to syslog.
+	  Send log messages to specified <file>.
 	- "standard":
 	  Send messages to standard output.
 
-- 
2.29.2

Re: [ndctl PATCH v4 1/4] cxl/monitor: Enable default_log and refactor sanity check

[Alison Schofield]

On Tue, Jul 11, 2023 at 07:53:41PM +0800, Li Zhijian wrote:
> The default_log(/var/log/cxl-monitor.log) should be used when no '-l'
> argument is specified in daemon mode, but it was not working at all.
> 
> Simplify the sanity checks so that the default log file is assigned
> correctly, and the behavior is consistent with the documentation.
> 
> Remove the unnecessary fix_filename() for monitor.log since
> parse_options_prefix() has done similar stuff if needed.

Reviewed-by: Alison Schofield <alison.schofield@intel.com>

> 
> Reviewed-by: Dave Jiang <dave.jiang@intel.com>
> Signed-off-by: Li Zhijian <lizhijian@fujitsu.com>
> ---
> V4: add reviewed tag and minor fixes: comment style and change log.
> V2: exchange order of previous patch1 and patch2 # Alison
>     a few commit log updated
> Signed-off-by: Li Zhijian <lizhijian@fujitsu.com>
> ---
>  cxl/monitor.c | 38 ++++++++++++++++++++------------------
>  1 file changed, 20 insertions(+), 18 deletions(-)
> 
> diff --git a/cxl/monitor.c b/cxl/monitor.c
> index e3469b9a4792..d8245ed8d0e9 100644
> --- a/cxl/monitor.c
> +++ b/cxl/monitor.c
> @@ -164,6 +164,7 @@ int cmd_monitor(int argc, const char **argv, struct cxl_ctx *ctx)
>  	};
>  	const char *prefix ="./";
>  	int rc = 0, i;
> +	const char *log;
>  
>  	argc = parse_options_prefix(argc, argv, prefix, options, u, 0);
>  	for (i = 0; i < argc; i++)
> @@ -171,32 +172,33 @@ int cmd_monitor(int argc, const char **argv, struct cxl_ctx *ctx)
>  	if (argc)
>  		usage_with_options(u, options);
>  
> +	/* sanity check */
> +	if (monitor.daemon && monitor.log && !strncmp(monitor.log, "./", 2)) {
> +		error("relative path or 'standard' are not compatible with daemon mode\n");
> +		return -EINVAL;
> +	}
> +
>  	log_init(&monitor.ctx, "cxl/monitor", "CXL_MONITOR_LOG");
> -	monitor.ctx.log_fn = log_standard;
> +	if (monitor.log)
> +		log = monitor.log;
> +	else
> +		log = monitor.daemon ? default_log : "./standard";
>  
>  	if (monitor.verbose)
>  		monitor.ctx.log_priority = LOG_DEBUG;
>  	else
>  		monitor.ctx.log_priority = LOG_INFO;
>  
> -	if (monitor.log) {
> -		if (strncmp(monitor.log, "./", 2) != 0)
> -			fix_filename(prefix, (const char **)&monitor.log);
> -		if (strncmp(monitor.log, "./standard", 10) == 0 && !monitor.daemon) {
> -			monitor.ctx.log_fn = log_standard;
> -		} else {
> -			const char *log = monitor.log;
> -
> -			if (!monitor.log)
> -				log = default_log;
> -			monitor.log_file = fopen(log, "a+");
> -			if (!monitor.log_file) {
> -				rc = -errno;
> -				error("open %s failed: %d\n", monitor.log, rc);
> -				goto out;
> -			}
> -			monitor.ctx.log_fn = log_file;
> +	if (strncmp(log, "./standard", 10) == 0)
> +		monitor.ctx.log_fn = log_standard;
> +	else {
> +		monitor.log_file = fopen(log, "a+");
> +		if (!monitor.log_file) {
> +			rc = -errno;
> +			error("open %s failed: %d\n", log, rc);
> +			goto out;
>  		}
> +		monitor.ctx.log_fn = log_file;
>  	}
>  
>  	if (monitor.daemon) {
> -- 
> 2.29.2
> 

Re: [ndctl PATCH v4 2/4] cxl/monitor: replace monitor.log_file with monitor.ctx.log_file

[Alison Schofield]

On Tue, Jul 11, 2023 at 07:53:42PM +0800, Li Zhijian wrote:
> Commit ba5825b0b7e0 ("ndctl/monitor: move common logging functions to util/log.c")
> have replaced monitor.log_file with monitor.ctx.log_file for
> ndctl-monitor, but for cxl-monitor, it forgot to do such work.
> 
> So where user specifies its own logfile, a segmentation fault will be
> trggered like below:
> 
>  # build/cxl/cxl monitor -l ./monitor.log
> Segmentation fault (core dumped)

Reviewed-by: Alison Schofield <alison.schofield@intel.com>


> 
> Fixes: 299f69f974a6 ("cxl/monitor: add a new monitor command for CXL trace events")
> Reviewed-by: Dave Jiang <dave.jiang@intel.com>
> Signed-off-by: Li Zhijian <lizhijian@fujitsu.com>
> ---
> V4: add reviewed tag
> V2: exchange order of previous patch1 and patch2 # Alison
>     a few commit log updated
> ---
>  cxl/monitor.c | 9 ++++-----
>  1 file changed, 4 insertions(+), 5 deletions(-)
> 
> diff --git a/cxl/monitor.c b/cxl/monitor.c
> index d8245ed8d0e9..e83455b63d35 100644
> --- a/cxl/monitor.c
> +++ b/cxl/monitor.c
> @@ -37,7 +37,6 @@ const char *default_log = "/var/log/cxl-monitor.log";
>  static struct monitor {
>  	const char *log;
>  	struct log_ctx ctx;
> -	FILE *log_file;
>  	bool human;
>  	bool verbose;
>  	bool daemon;
> @@ -192,8 +191,8 @@ int cmd_monitor(int argc, const char **argv, struct cxl_ctx *ctx)
>  	if (strncmp(log, "./standard", 10) == 0)
>  		monitor.ctx.log_fn = log_standard;
>  	else {
> -		monitor.log_file = fopen(log, "a+");
> -		if (!monitor.log_file) {
> +		monitor.ctx.log_file = fopen(log, "a+");
> +		if (!monitor.ctx.log_file) {
>  			rc = -errno;
>  			error("open %s failed: %d\n", log, rc);
>  			goto out;
> @@ -212,7 +211,7 @@ int cmd_monitor(int argc, const char **argv, struct cxl_ctx *ctx)
>  	rc = monitor_event(ctx);
>  
>  out:
> -	if (monitor.log_file)
> -		fclose(monitor.log_file);
> +	if (monitor.ctx.log_file)
> +		fclose(monitor.ctx.log_file);
>  	return rc;
>  }
> -- 
> 2.29.2
> 

Re: [ndctl PATCH v4 3/4] ndctl: use strcmp for reserved word in monitor commands

[Alison Schofield]

On Tue, Jul 11, 2023 at 07:53:43PM +0800, Li Zhijian wrote:
> According to the tool's documentation, when '-l standard' is specified,
> log would be output to the stdout. But since it's using strncmp(a, b, 10)
> to compare the former 10 characters, it will also wrongly detect a filename
> starting with a substring 'standard' as stdout.
> 
> For example:
> $ cxl monitor -l standard.log
> 
> User is most likely want to save log to ./standard.log instead of stdout.

Reviewed-by: Alison Schofield <alison.schofield@intel.com>

> 
> Reviewed-by: Dave Jiang <dave.jiang@intel.com>
> Signed-off-by: Li Zhijian <lizhijian@fujitsu.com>
> ---
> V4: combine ndctl/monitor to one patch
> V3: Improve commit log # Dave
> V2: commit log updated # Dave
> Signed-off-by: Li Zhijian <lizhijian@fujitsu.com>
> ---
>  cxl/monitor.c   | 2 +-
>  ndctl/monitor.c | 4 ++--
>  2 files changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/cxl/monitor.c b/cxl/monitor.c
> index e83455b63d35..a85452a4dc82 100644
> --- a/cxl/monitor.c
> +++ b/cxl/monitor.c
> @@ -188,7 +188,7 @@ int cmd_monitor(int argc, const char **argv, struct cxl_ctx *ctx)
>  	else
>  		monitor.ctx.log_priority = LOG_INFO;
>  
> -	if (strncmp(log, "./standard", 10) == 0)
> +	if (strcmp(log, "./standard") == 0)
>  		monitor.ctx.log_fn = log_standard;
>  	else {
>  		monitor.ctx.log_file = fopen(log, "a+");
> diff --git a/ndctl/monitor.c b/ndctl/monitor.c
> index 89903def63d4..bd8a74863476 100644
> --- a/ndctl/monitor.c
> +++ b/ndctl/monitor.c
> @@ -610,9 +610,9 @@ int cmd_monitor(int argc, const char **argv, struct ndctl_ctx *ctx)
>  	if (monitor.log) {
>  		if (strncmp(monitor.log, "./", 2) != 0)
>  			fix_filename(prefix, (const char **)&monitor.log);
> -		if (strncmp(monitor.log, "./syslog", 8) == 0)
> +		if (strcmp(monitor.log, "./syslog") == 0)
>  			monitor.ctx.log_fn = log_syslog;
> -		else if (strncmp(monitor.log, "./standard", 10) == 0)
> +		else if (strcmp(monitor.log, "./standard") == 0)
>  			monitor.ctx.log_fn = log_standard;
>  		else {
>  			monitor.ctx.log_file = fopen(monitor.log, "a+");
> -- 
> 2.29.2
> 

Re: [ndctl PATCH v4 4/4] Documentation/cxl/cxl-monitor.txt: Fix inaccurate description

[Alison Schofield]

On Tue, Jul 11, 2023 at 07:53:44PM +0800, Li Zhijian wrote:
> No syslog is supported by cxl-monitor

Reviewed-by: Alison Schofield <alison.schofield@intel.com>

> 
> Reviewed-by: Dave Jiang <dave.jiang@intel.com>
> Signed-off-by: Li Zhijian <lizhijian@fujitsu.com>
> ---
> V2: add Reviewed-by tag
> ---
>  Documentation/cxl/cxl-monitor.txt | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/Documentation/cxl/cxl-monitor.txt b/Documentation/cxl/cxl-monitor.txt
> index 3fc992e4d4d9..c284099f16c3 100644
> --- a/Documentation/cxl/cxl-monitor.txt
> +++ b/Documentation/cxl/cxl-monitor.txt
> @@ -39,8 +39,7 @@ OPTIONS
>  --log=::
>  	Send log messages to the specified destination.
>  	- "<file>":
> -	  Send log messages to specified <file>. When fopen() is not able
> -	  to open <file>, log messages will be forwarded to syslog.
> +	  Send log messages to specified <file>.
>  	- "standard":
>  	  Send messages to standard output.
>  
> -- 
> 2.29.2
>