[ndctl PATCH 0/6] cxl/monitor and ndctl/monitor fixes

[ndctl PATCH 0/6] cxl/monitor and ndctl/monitor fixes

[Li Zhijian]

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 works more accurately

Li Zhijian (6):
  cxl/monitor: Fix monitor not working
  cxl/monitor: compare the whole filename with reserved words
  cxl/monitor: Enable default_log and refactor sanity check
  cxl/monitor: always log started message
  Documentation/cxl/cxl-monitor.txt: Fix inaccurate description
  ndctl/monitor: compare the whole filename with reserved words

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

-- 
2.29.2

[ndctl PATCH 1/6] cxl/monitor: Fix monitor not working

[Li Zhijian]

It looks that someone forgot to rewrite this part after
ba5825b0b7e0 ("ndctl/monitor: move common logging functions to util/log.c")

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

Fixes: 299f69f974a6 ("cxl/monitor: add a new monitor command for CXL trace events")
Signed-off-by: Li Zhijian <lizhijian@fujitsu.com>
---
 cxl/monitor.c | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/cxl/monitor.c b/cxl/monitor.c
index e3469b9a4792..4043928db3ef 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;
@@ -189,8 +188,8 @@ int cmd_monitor(int argc, const char **argv, struct cxl_ctx *ctx)
 
 			if (!monitor.log)
 				log = default_log;
-			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", monitor.log, rc);
 				goto out;
@@ -210,7 +209,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 2/6] cxl/monitor: compare the whole filename with reserved words

[Li Zhijian]

For example:
$ cxl monitor -l standard.log

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

Signed-off-by: Li Zhijian <lizhijian@fujitsu.com>
---
 cxl/monitor.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/cxl/monitor.c b/cxl/monitor.c
index 4043928db3ef..842e54b186ab 100644
--- a/cxl/monitor.c
+++ b/cxl/monitor.c
@@ -181,7 +181,8 @@ int cmd_monitor(int argc, const char **argv, struct cxl_ctx *ctx)
 	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) {
+
+		if (strcmp(monitor.log, "./standard") == 0 && !monitor.daemon) {
 			monitor.ctx.log_fn = log_standard;
 		} else {
 			const char *log = monitor.log;
-- 
2.29.2

[ndctl PATCH 3/6] cxl/monitor: Enable default_log and refactor sanity check

[Li Zhijian]

The default_log is not working at all. Simply the sanity check and
re-enable default log file so that it can be consistent with the
document.

Please note that i also removed following addition stuff, since we have
added this prefix if needed during parsing the FILENAME.
if (strncmp(monitor.log, "./", 2) != 0)
    fix_filename(prefix, (const char **)&monitor.log);

Signed-off-by: Li Zhijian <lizhijian@fujitsu.com>
---
 cxl/monitor.c | 41 +++++++++++++++++++++--------------------
 1 file changed, 21 insertions(+), 20 deletions(-)

diff --git a/cxl/monitor.c b/cxl/monitor.c
index 842e54b186ab..139506aed85a 100644
--- a/cxl/monitor.c
+++ b/cxl/monitor.c
@@ -163,6 +163,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++)
@@ -170,32 +171,32 @@ int cmd_monitor(int argc, const char **argv, struct cxl_ctx *ctx)
 	if (argc)
 		usage_with_options(u, options);
 
-	log_init(&monitor.ctx, "cxl/monitor", "CXL_MONITOR_LOG");
-	monitor.ctx.log_fn = log_standard;
+	// sanity check
+	if (monitor.daemon && monitor.log && !strncmp(monitor.log, "./", 2)) {
+		error("standard or relative path for <file> will not work for daemon mode\n");
+		return -EINVAL;
+	}
+
+	if (monitor.log)
+		log = monitor.log;
+	else
+		log = monitor.daemon ? default_log : "./standard";
 
+	log_init(&monitor.ctx, "cxl/monitor", "CXL_MONITOR_LOG");
 	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 (strcmp(monitor.log, "./standard") == 0 && !monitor.daemon) {
-			monitor.ctx.log_fn = log_standard;
-		} else {
-			const char *log = monitor.log;
-
-			if (!monitor.log)
-				log = default_log;
-			monitor.ctx.log_file = fopen(log, "a+");
-			if (!monitor.ctx.log_file) {
-				rc = -errno;
-				error("open %s failed: %d\n", monitor.log, rc);
-				goto out;
-			}
-			monitor.ctx.log_fn = log_file;
+	if (strcmp(log, "./standard") == 0)
+		monitor.ctx.log_fn = log_standard;
+	else {
+		monitor.ctx.log_fn = 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;
 		}
 	}
 
-- 
2.29.2

[ndctl PATCH 4/6] cxl/monitor: always log started message

[Li Zhijian]

Tell people monitor is starting

Signed-off-by: Li Zhijian <lizhijian@fujitsu.com>
---
 cxl/monitor.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/cxl/monitor.c b/cxl/monitor.c
index 139506aed85a..6761f3bb97af 100644
--- a/cxl/monitor.c
+++ b/cxl/monitor.c
@@ -205,8 +205,8 @@ int cmd_monitor(int argc, const char **argv, struct cxl_ctx *ctx)
 			err(&monitor, "daemon start failed\n");
 			goto out;
 		}
-		info(&monitor, "cxl monitor daemon started.\n");
 	}
+	info(&monitor, "cxl monitor started.\n");
 
 	rc = monitor_event(ctx);
 
-- 
2.29.2

[ndctl PATCH 5/6] Documentation/cxl/cxl-monitor.txt: Fix inaccurate description

[Li Zhijian]

No syslog is supported by cxl-monitor

Signed-off-by: Li Zhijian <lizhijian@fujitsu.com>
---
 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

[ndctl PATCH 6/6] ndctl/monitor: compare the whole filename with reserved words

[Li Zhijian]

For example:
$ ndctl monitor -l standard.log
User is most likely want to save log to ./standard.log instead of stdout.

Signed-off-by: Li Zhijian <lizhijian@fujitsu.com>
---
 ndctl/monitor.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

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 1/6] cxl/monitor: Fix monitor not working

[Xiao Yang (Fujitsu)]

On 2023/5/13 22:20, Li Zhijian wrote:
> It looks that someone forgot to rewrite this part after
> ba5825b0b7e0 ("ndctl/monitor: move common logging functions to util/log.c")
Hi zhijian,

It's fine to update this part.

> 
>   # build/cxl/cxl monitor -l ./monitor.log
> Segmentation fault (core dumped)
I cannot reproduce the issue and current code looks good.
Did I miss something?

Best Regard
Xiao Yang
> 
> Fixes: 299f69f974a6 ("cxl/monitor: add a new monitor command for CXL trace events")
> Signed-off-by: Li Zhijian<lizhijian@fujitsu.com>

Re: [ndctl PATCH 1/6] cxl/monitor: Fix monitor not working

[Zhijian Li (Fujitsu)]

On 15/05/2023 15:17, Yang, Xiao/杨 晓 wrote:
> On 2023/5/13 22:20, Li Zhijian wrote:
>> It looks that someone forgot to rewrite this part after
>> ba5825b0b7e0 ("ndctl/monitor: move common logging functions to util/log.c")
> Hi zhijian,
> 
> It's fine to update this part.
> 
>>
>>    # build/cxl/cxl monitor -l ./monitor.log
>> Segmentation fault (core dumped)
> I cannot reproduce the issue and current code looks good.
> Did I miss something?

Maybe your monitor haven't received any events, so no messages are really logged to <file>.



> 
> Best Regard
> Xiao Yang
>>
>> Fixes: 299f69f974a6 ("cxl/monitor: add a new monitor command for CXL trace events")
>> Signed-off-by: Li Zhijian<lizhijian@fujitsu.com>

Re: [ndctl PATCH 1/6] cxl/monitor: Fix monitor not working

[Dave Jiang]

On 5/13/23 7:20 AM, Li Zhijian wrote:
> It looks that someone forgot to rewrite this part after
> ba5825b0b7e0 ("ndctl/monitor: move common logging functions to util/log.c")
> 
>   # build/cxl/cxl monitor -l ./monitor.log
> Segmentation fault (core dumped)
> 
> Fixes: 299f69f974a6 ("cxl/monitor: add a new monitor command for CXL trace events")
> Signed-off-by: Li Zhijian <lizhijian@fujitsu.com>

Reviewed-by: Dave Jiang <dave.jiang@intel.com>

> ---
>   cxl/monitor.c | 9 ++++-----
>   1 file changed, 4 insertions(+), 5 deletions(-)
> 
> diff --git a/cxl/monitor.c b/cxl/monitor.c
> index e3469b9a4792..4043928db3ef 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;
> @@ -189,8 +188,8 @@ int cmd_monitor(int argc, const char **argv, struct cxl_ctx *ctx)
>   
>   			if (!monitor.log)
>   				log = default_log;
> -			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", monitor.log, rc);
>   				goto out;
> @@ -210,7 +209,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;
>   }

Re: [ndctl PATCH 2/6] cxl/monitor: compare the whole filename with reserved words

[Dave Jiang]

On 5/13/23 7:20 AM, Li Zhijian wrote:
> For example:
> $ cxl monitor -l standard.log
> 
> User is most likely want to save log to ./standard.log instead of stdout.
> 
> Signed-off-by: Li Zhijian <lizhijian@fujitsu.com>
> ---
>   cxl/monitor.c | 3 ++-
>   1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/cxl/monitor.c b/cxl/monitor.c
> index 4043928db3ef..842e54b186ab 100644
> --- a/cxl/monitor.c
> +++ b/cxl/monitor.c
> @@ -181,7 +181,8 @@ int cmd_monitor(int argc, const char **argv, struct cxl_ctx *ctx)
>   	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) {
> +
> +		if (strcmp(monitor.log, "./standard") == 0 && !monitor.daemon) {

The code change doesn't match the commit log. Here it just changed from 
strncmp() to strcmp(). Please explain what's going on here.

>   			monitor.ctx.log_fn = log_standard;
>   		} else {
>   			const char *log = monitor.log;

Re: [ndctl PATCH 4/6] cxl/monitor: always log started message

[Dave Jiang]

On 5/13/23 7:20 AM, Li Zhijian wrote:
> Tell people monitor is starting

Commit log terse and non-informative. Please describe what was the past 
behavior and what is the new behavior. Thank you.
> 
> Signed-off-by: Li Zhijian <lizhijian@fujitsu.com>
> ---
>   cxl/monitor.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/cxl/monitor.c b/cxl/monitor.c
> index 139506aed85a..6761f3bb97af 100644
> --- a/cxl/monitor.c
> +++ b/cxl/monitor.c
> @@ -205,8 +205,8 @@ int cmd_monitor(int argc, const char **argv, struct cxl_ctx *ctx)
>   			err(&monitor, "daemon start failed\n");
>   			goto out;
>   		}
> -		info(&monitor, "cxl monitor daemon started.\n");
>   	}
> +	info(&monitor, "cxl monitor started.\n");
>   
>   	rc = monitor_event(ctx);
>   

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

[Dave Jiang]

On 5/13/23 7:20 AM, Li Zhijian wrote:
> No syslog is supported by cxl-monitor
> 
> Signed-off-by: Li Zhijian <lizhijian@fujitsu.com>

Reviewed-by: Dave Jiang <davejiang@intel.com>

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

Re: [ndctl PATCH 6/6] ndctl/monitor: compare the whole filename with reserved words

[Dave Jiang]

On 5/13/23 7:20 AM, Li Zhijian wrote:
> For example:
> $ ndctl monitor -l standard.log
> User is most likely want to save log to ./standard.log instead of stdout.

Same comment as patch 1. Commit log does not match code change. Please 
explain.

> 
> Signed-off-by: Li Zhijian <lizhijian@fujitsu.com>
> ---
>   ndctl/monitor.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
> 
> 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+");

Re: [ndctl PATCH 0/6] cxl/monitor and ndctl/monitor fixes

[Dave Jiang]

On 5/13/23 7:20 AM, Li Zhijian wrote:
> 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 works more accurately
> 
> Li Zhijian (6):
>    cxl/monitor: Fix monitor not working
>    cxl/monitor: compare the whole filename with reserved words
>    cxl/monitor: Enable default_log and refactor sanity check
>    cxl/monitor: always log started message
>    Documentation/cxl/cxl-monitor.txt: Fix inaccurate description
>    ndctl/monitor: compare the whole filename with reserved words
> 
>   Documentation/cxl/cxl-monitor.txt |  3 +-
>   cxl/monitor.c                     | 47 ++++++++++++++++---------------
>   ndctl/monitor.c                   |  4 +--
>   3 files changed, 27 insertions(+), 27 deletions(-)
> 

Please cc linux-cxl@vger.kernel.org as well for future revs since this 
impacts CXL CLI.

Re: [ndctl PATCH 1/6] cxl/monitor: Fix monitor not working

[Alison Schofield]

On Sat, May 13, 2023 at 10:20:33PM +0800, Li Zhijian wrote:
> It looks that someone forgot to rewrite this part after
> ba5825b0b7e0 ("ndctl/monitor: move common logging functions to util/log.c")
> 
>  # build/cxl/cxl monitor -l ./monitor.log
> Segmentation fault (core dumped)
> 
> Fixes: 299f69f974a6 ("cxl/monitor: add a new monitor command for CXL trace events")
> Signed-off-by: Li Zhijian <lizhijian@fujitsu.com>

Hi Zhijian,

A more explicit commit message would be helpful, something like:
cxl/monitor: replace monitor.log_file with monitor.ctx.log_file

I understand your "It looks that someone forgot...", and it may be
true, but that is only pertinent in the commit log if it actually
caused the problem. In this case, it didn't because it merged before
the patch in your Fixes Tag.

I'd expect this commit log to include:
1) why it's broken - inconsistent use of 'log_file'
2) impact on user
3) steps to reproduce

And, a little bit more below...


> ---
>  cxl/monitor.c | 9 ++++-----
>  1 file changed, 4 insertions(+), 5 deletions(-)
> 
> diff --git a/cxl/monitor.c b/cxl/monitor.c
> index e3469b9a4792..4043928db3ef 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;
> @@ -189,8 +188,8 @@ int cmd_monitor(int argc, const char **argv, struct cxl_ctx *ctx)
>  
>  			if (!monitor.log)
>  				log = default_log;

Can the above 'if' ever happen here?  Seems we checked for monitor.log
a few lines above?

Thanks,
Alison


> -			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", monitor.log, rc);
>  				goto out;
> @@ -210,7 +209,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 3/6] cxl/monitor: Enable default_log and refactor sanity check

[Alison Schofield]

On Sat, May 13, 2023 at 10:20:35PM +0800, Li Zhijian wrote:
> The default_log is not working at all. Simply the sanity check and
> re-enable default log file so that it can be consistent with the
> document.
> 
> Please note that i also removed following addition stuff, since we have
> added this prefix if needed during parsing the FILENAME.
> if (strncmp(monitor.log, "./", 2) != 0)
>     fix_filename(prefix, (const char **)&monitor.log);

Hi Zhijian,

I reviewed the first patch, without looking at all the patches in
the set. It seems like the set touches cmd_monitor() at least 2
times, and then dives into refactoring it.

I'm confused. I think I could be less confused with a cover letter
explaining the flow of this set. Maybe the flow of the set can be
improved. It seems they are presented in the order that you discovered
an issue, and that may not be the cleanest way to present them for
merging.

Thanks,
Alison

> 
> Signed-off-by: Li Zhijian <lizhijian@fujitsu.com>
> ---
>  cxl/monitor.c | 41 +++++++++++++++++++++--------------------
>  1 file changed, 21 insertions(+), 20 deletions(-)
> 
> diff --git a/cxl/monitor.c b/cxl/monitor.c
> index 842e54b186ab..139506aed85a 100644
> --- a/cxl/monitor.c
> +++ b/cxl/monitor.c
> @@ -163,6 +163,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++)
> @@ -170,32 +171,32 @@ int cmd_monitor(int argc, const char **argv, struct cxl_ctx *ctx)
>  	if (argc)
>  		usage_with_options(u, options);
>  
> -	log_init(&monitor.ctx, "cxl/monitor", "CXL_MONITOR_LOG");
> -	monitor.ctx.log_fn = log_standard;
> +	// sanity check
> +	if (monitor.daemon && monitor.log && !strncmp(monitor.log, "./", 2)) {
> +		error("standard or relative path for <file> will not work for daemon mode\n");
> +		return -EINVAL;
> +	}
> +
> +	if (monitor.log)
> +		log = monitor.log;
> +	else
> +		log = monitor.daemon ? default_log : "./standard";
>  
> +	log_init(&monitor.ctx, "cxl/monitor", "CXL_MONITOR_LOG");
>  	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 (strcmp(monitor.log, "./standard") == 0 && !monitor.daemon) {
> -			monitor.ctx.log_fn = log_standard;
> -		} else {
> -			const char *log = monitor.log;
> -
> -			if (!monitor.log)
> -				log = default_log;
> -			monitor.ctx.log_file = fopen(log, "a+");
> -			if (!monitor.ctx.log_file) {
> -				rc = -errno;
> -				error("open %s failed: %d\n", monitor.log, rc);
> -				goto out;
> -			}
> -			monitor.ctx.log_fn = log_file;
> +	if (strcmp(log, "./standard") == 0)
> +		monitor.ctx.log_fn = log_standard;
> +	else {
> +		monitor.ctx.log_fn = 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;
>  		}
>  	}
>  
> -- 
> 2.29.2
> 
> 

Re: [ndctl PATCH 2/6] cxl/monitor: compare the whole filename with reserved words

[Zhijian Li (Fujitsu)]

Dave


On 20/05/2023 01:31, Dave Jiang wrote:
> 
> 
> On 5/13/23 7:20 AM, Li Zhijian wrote:
>> For example:
>> $ cxl monitor -l standard.log
>>
>> User is most likely want to save log to ./standard.log instead of stdout.
>>
>> Signed-off-by: Li Zhijian <lizhijian@fujitsu.com>
>> ---
>>   cxl/monitor.c | 3 ++-
>>   1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/cxl/monitor.c b/cxl/monitor.c
>> index 4043928db3ef..842e54b186ab 100644
>> --- a/cxl/monitor.c
>> +++ b/cxl/monitor.c
>> @@ -181,7 +181,8 @@ int cmd_monitor(int argc, const char **argv, struct cxl_ctx *ctx)
>>       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) {
>> +
>> +        if (strcmp(monitor.log, "./standard") == 0 && !monitor.daemon) {
> 
> The code change doesn't match the commit log. Here it just changed from strncmp() to strcmp(). Please explain what's going on here.
> 


Okay, i will update more in the commit log. something like:

     cxl/monitor: use strcmp to compare the reserved word
     
     According to its document, when '-l standard' is specified, log would be
     output to the stdout. But actually, since it's using strncmp(a, b, 10)
     to compare the former 10 characters, it will also wrongly treat a filename
     starting with a substring 'standard' to stdout.
     
     For example:
     $ cxl monitor -l standard.log
     
     User is most likely want to save log to ./standard.log instead of stdout.
     
     Signed-off-by: Li Zhijian <lizhijian@fujitsu.com>
     ---
     V2: commit log updated # Dave


Thanks
Zhijian



>>               monitor.ctx.log_fn = log_standard;
>>           } else {
>>               const char *log = monitor.log;
> 

Re: [ndctl PATCH 3/6] cxl/monitor: Enable default_log and refactor sanity check

[Zhijian Li (Fujitsu)]

Alison


On 22/05/2023 04:41, Alison Schofield wrote:
> On Sat, May 13, 2023 at 10:20:35PM +0800, Li Zhijian wrote:
>> The default_log is not working at all. Simply the sanity check and
>> re-enable default log file so that it can be consistent with the
>> document.
>>
>> Please note that i also removed following addition stuff, since we have
>> added this prefix if needed during parsing the FILENAME.
>> if (strncmp(monitor.log, "./", 2) != 0)
>>      fix_filename(prefix, (const char **)&monitor.log);
> 
> Hi Zhijian,
> 
> I reviewed the first patch, without looking at all the patches in
> the set. It seems like the set touches cmd_monitor() at least 2
> times, and then dives into refactoring it.
> 
> I'm confused. I think I could be less confused with a cover letter
> explaining the flow of this set. Maybe the flow of the set can be
> improved. It seems they are presented in the order that you discovered
> an issue,

Yes, that's true.

  and that may not be the cleanest way to present them for
> merging.
> 


I will exchange the order of previous patch1 and patch2 in V2 and update the cover letter as well.


Thanks
Zhijian

> Thanks,
> Alison
> 
>>
>> Signed-off-by: Li Zhijian <lizhijian@fujitsu.com>
>> ---
>>   cxl/monitor.c | 41 +++++++++++++++++++++--------------------
>>   1 file changed, 21 insertions(+), 20 deletions(-)
>>
>> diff --git a/cxl/monitor.c b/cxl/monitor.c
>> index 842e54b186ab..139506aed85a 100644
>> --- a/cxl/monitor.c
>> +++ b/cxl/monitor.c
>> @@ -163,6 +163,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++)
>> @@ -170,32 +171,32 @@ int cmd_monitor(int argc, const char **argv, struct cxl_ctx *ctx)
>>   	if (argc)
>>   		usage_with_options(u, options);
>>   
>> -	log_init(&monitor.ctx, "cxl/monitor", "CXL_MONITOR_LOG");
>> -	monitor.ctx.log_fn = log_standard;
>> +	// sanity check
>> +	if (monitor.daemon && monitor.log && !strncmp(monitor.log, "./", 2)) {
>> +		error("standard or relative path for <file> will not work for daemon mode\n");
>> +		return -EINVAL;
>> +	}
>> +
>> +	if (monitor.log)
>> +		log = monitor.log;
>> +	else
>> +		log = monitor.daemon ? default_log : "./standard";
>>   
>> +	log_init(&monitor.ctx, "cxl/monitor", "CXL_MONITOR_LOG");
>>   	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 (strcmp(monitor.log, "./standard") == 0 && !monitor.daemon) {
>> -			monitor.ctx.log_fn = log_standard;
>> -		} else {
>> -			const char *log = monitor.log;
>> -
>> -			if (!monitor.log)
>> -				log = default_log;
>> -			monitor.ctx.log_file = fopen(log, "a+");
>> -			if (!monitor.ctx.log_file) {
>> -				rc = -errno;
>> -				error("open %s failed: %d\n", monitor.log, rc);
>> -				goto out;
>> -			}
>> -			monitor.ctx.log_fn = log_file;
>> +	if (strcmp(log, "./standard") == 0)
>> +		monitor.ctx.log_fn = log_standard;
>> +	else {
>> +		monitor.ctx.log_fn = 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;
>>   		}
>>   	}
>>   
>> -- 
>> 2.29.2
>>
>>

Re: [ndctl PATCH 4/6] cxl/monitor: always log started message

[Zhijian Li (Fujitsu)]

On 20/05/2023 01:43, Dave Jiang wrote:
> 
> 
> On 5/13/23 7:20 AM, Li Zhijian wrote:
>> Tell people monitor is starting
> 
> Commit log terse and non-informative. Please describe what was the past behavior and what is the new behavior. Thank you.

Thanks for your comments, i will update it in V2

Thanks
Zhijian


>>
>> Signed-off-by: Li Zhijian <lizhijian@fujitsu.com>
>> ---
>>   cxl/monitor.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/cxl/monitor.c b/cxl/monitor.c
>> index 139506aed85a..6761f3bb97af 100644
>> --- a/cxl/monitor.c
>> +++ b/cxl/monitor.c
>> @@ -205,8 +205,8 @@ int cmd_monitor(int argc, const char **argv, struct cxl_ctx *ctx)
>>               err(&monitor, "daemon start failed\n");
>>               goto out;
>>           }
>> -        info(&monitor, "cxl monitor daemon started.\n");
>>       }
>> +    info(&monitor, "cxl monitor started.\n");
>>       rc = monitor_event(ctx);

Re: [ndctl PATCH 0/6] cxl/monitor and ndctl/monitor fixes

[Zhijian Li (Fujitsu)]

On 20/05/2023 01:47, Dave Jiang wrote:
> 
> 
> On 5/13/23 7:20 AM, Li Zhijian wrote:
>> 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 works more accurately
>>
>> Li Zhijian (6):
>>    cxl/monitor: Fix monitor not working
>>    cxl/monitor: compare the whole filename with reserved words
>>    cxl/monitor: Enable default_log and refactor sanity check
>>    cxl/monitor: always log started message
>>    Documentation/cxl/cxl-monitor.txt: Fix inaccurate description
>>    ndctl/monitor: compare the whole filename with reserved words
>>
>>   Documentation/cxl/cxl-monitor.txt |  3 +-
>>   cxl/monitor.c                     | 47 ++++++++++++++++---------------
>>   ndctl/monitor.c                   |  4 +--
>>   3 files changed, 27 insertions(+), 27 deletions(-)
>>
> 
> Please cc linux-cxl@vger.kernel.org as well for future revs since this impacts CXL CLI.
Yeah

I have double confirmed recipient in this repo by 'git grep @', look we have to document it
in the CONTRIBUTING.md as well :)

Re: [ndctl PATCH 2/6] cxl/monitor: compare the whole filename with reserved words

[Dave Jiang]

On 5/21/23 11:08 PM, Zhijian Li (Fujitsu) wrote:
> Dave
> 
> 
> On 20/05/2023 01:31, Dave Jiang wrote:
>>
>>
>> On 5/13/23 7:20 AM, Li Zhijian wrote:
>>> For example:
>>> $ cxl monitor -l standard.log
>>>
>>> User is most likely want to save log to ./standard.log instead of stdout.
>>>
>>> Signed-off-by: Li Zhijian <lizhijian@fujitsu.com>
>>> ---
>>>    cxl/monitor.c | 3 ++-
>>>    1 file changed, 2 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/cxl/monitor.c b/cxl/monitor.c
>>> index 4043928db3ef..842e54b186ab 100644
>>> --- a/cxl/monitor.c
>>> +++ b/cxl/monitor.c
>>> @@ -181,7 +181,8 @@ int cmd_monitor(int argc, const char **argv, struct cxl_ctx *ctx)
>>>        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) {
>>> +
>>> +        if (strcmp(monitor.log, "./standard") == 0 && !monitor.daemon) {
>>
>> The code change doesn't match the commit log. Here it just changed from strncmp() to strcmp(). Please explain what's going on here.
>>
> 
> 
> Okay, i will update more in the commit log. something like:
> 
>       cxl/monitor: use strcmp to compare the reserved word
>       
>       According to its document, when '-l standard' is specified, log would be

s/its document/the tool's documentation/

>       output to the stdout. But actually, since it's using strncmp(a, b, 10)

s/But actually, since/But since/

>       to compare the former 10 characters, it will also wrongly treat a filename

s/treat/detect/

>       starting with a substring 'standard' to stdout.

s/to/as/

>       
>       For example:
>       $ cxl monitor -l standard.log
>       
>       User is most likely want to save log to ./standard.log instead of stdout.
>       
>       Signed-off-by: Li Zhijian <lizhijian@fujitsu.com>
>       ---
>       V2: commit log updated # Dave

This makes it significantly clearer. Thank you.


> 
> 
> Thanks
> Zhijian
> 
> 
> 
>>>                monitor.ctx.log_fn = log_standard;
>>>            } else {
>>>                const char *log = monitor.log;

Re: [ndctl PATCH 0/6] cxl/monitor and ndctl/monitor fixes

[Dave Jiang]

On 5/21/23 11:25 PM, Zhijian Li (Fujitsu) wrote:
> 
> 
> On 20/05/2023 01:47, Dave Jiang wrote:
>>
>>
>> On 5/13/23 7:20 AM, Li Zhijian wrote:
>>> 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 works more accurately
>>>
>>> Li Zhijian (6):
>>>     cxl/monitor: Fix monitor not working
>>>     cxl/monitor: compare the whole filename with reserved words
>>>     cxl/monitor: Enable default_log and refactor sanity check
>>>     cxl/monitor: always log started message
>>>     Documentation/cxl/cxl-monitor.txt: Fix inaccurate description
>>>     ndctl/monitor: compare the whole filename with reserved words
>>>
>>>    Documentation/cxl/cxl-monitor.txt |  3 +-
>>>    cxl/monitor.c                     | 47 ++++++++++++++++---------------
>>>    ndctl/monitor.c                   |  4 +--
>>>    3 files changed, 27 insertions(+), 27 deletions(-)
>>>
>>
>> Please cc linux-cxl@vger.kernel.org as well for future revs since this impacts CXL CLI.
> Yeah
> 
> I have double confirmed recipient in this repo by 'git grep @', look we have to document it
> in the CONTRIBUTING.md as well :)

Please feel free to submit a patch and rectify this. :)

Re: [ndctl PATCH 2/6] cxl/monitor: compare the whole filename with reserved words

[Zhijian Li (Fujitsu)]

On 22/05/2023 23:15, Dave Jiang wrote:
> 
> 
> On 5/21/23 11:08 PM, Zhijian Li (Fujitsu) wrote:
>> Dave
>>
>>
>> On 20/05/2023 01:31, Dave Jiang wrote:
>>>
>>>
>>> On 5/13/23 7:20 AM, Li Zhijian wrote:
>>>> For example:
>>>> $ cxl monitor -l standard.log
>>>>
>>>> User is most likely want to save log to ./standard.log instead of stdout.
>>>>
>>>> Signed-off-by: Li Zhijian <lizhijian@fujitsu.com>
>>>> ---
>>>>    cxl/monitor.c | 3 ++-
>>>>    1 file changed, 2 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/cxl/monitor.c b/cxl/monitor.c
>>>> index 4043928db3ef..842e54b186ab 100644
>>>> --- a/cxl/monitor.c
>>>> +++ b/cxl/monitor.c
>>>> @@ -181,7 +181,8 @@ int cmd_monitor(int argc, const char **argv, struct cxl_ctx *ctx)
>>>>        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) {
>>>> +
>>>> +        if (strcmp(monitor.log, "./standard") == 0 && !monitor.daemon) {
>>>
>>> The code change doesn't match the commit log. Here it just changed from strncmp() to strcmp(). Please explain what's going on here.
>>>
>>
>>
>> Okay, i will update more in the commit log. something like:
>>
>>       cxl/monitor: use strcmp to compare the reserved word
>>       According to its document, when '-l standard' is specified, log would be
> 
> s/its document/the tool's documentation/
> 
>>       output to the stdout. But actually, since it's using strncmp(a, b, 10)
> 
> s/But actually, since/But since/
> 
>>       to compare the former 10 characters, it will also wrongly treat a filename
> 
> s/treat/detect/
> 
>>       starting with a substring 'standard' to stdout.
> 
> s/to/as/
> 
>>       For example:
>>       $ cxl monitor -l standard.log
>>       User is most likely want to save log to ./standard.log instead of stdout.
>>       Signed-off-by: Li Zhijian <lizhijian@fujitsu.com>
>>       ---
>>       V2: commit log updated # Dave
> 
> This makes it significantly clearer. Thank you.


Sorry for the delay. your comments are good to me. thank you very much.
I have addressed them in my local V3 branch which will be sent if there is no more
comments to my V2 set in next week.


Thanks
Zhijian



> 
> 
>>
>>
>> Thanks
>> Zhijian
>>
>>
>>
>>>>                monitor.ctx.log_fn = log_standard;
>>>>            } else {
>>>>                const char *log = monitor.log;