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

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

[Li Zhijian]

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.

Li Zhijian (6):
  cxl/monitor: Enable default_log and refactor sanity check
  cxl/monitor: replace monitor.log_file with monitor.ctx.log_file
  cxl/monitor: use strcmp to compare the reserved word
  cxl/monitor: always log started message
  Documentation/cxl/cxl-monitor.txt: Fix inaccurate description
  ndctl/monitor: use strcmp to compare the reserved word

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

-- 
2.29.2

[ndctl PATCH v3 1/6] 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.

Here we assigned it a default log per its arguments, and simplify the
sanity check 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 in
parse_options_prefix().
if (strncmp(monitor.log, "./", 2) != 0)
    fix_filename(prefix, (const char **)&monitor.log);

Signed-off-by: Li Zhijian <lizhijian@fujitsu.com>
---
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..c6df2bad3c53 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("standard or relative path for <file> will not work for 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 v3 2/6] 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")
Signed-off-by: Li Zhijian <lizhijian@fujitsu.com>
---
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 c6df2bad3c53..f0e3c4c3f45c 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 v3 3/6] cxl/monitor: use strcmp to compare the reserved word

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

Signed-off-by: Li Zhijian <lizhijian@fujitsu.com>
---
V3: Improve commit log # Dave
V2: commit log updated # Dave
---
 cxl/monitor.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/cxl/monitor.c b/cxl/monitor.c
index f0e3c4c3f45c..179646562187 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+");
-- 
2.29.2

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

[Li Zhijian]

Tell people monitor is starting rather only daemon mode will log this
message before. It's a minor fix so that whatever stdout or logfile
is specified, people can see a *normal* message.

After this patch
 # cxl monitor
 cxl monitor started.
 ^C
 # cxl monitor -l standard.log
 ^C
 # cat standard.log
 [1684735993.704815571] [818499] cxl monitor started.
 # cxl monitor --daemon -l /var/log/daemon.log
 # cat /var/log/daemon.log
 [1684736075.817150494] [818509] cxl monitor started.

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

diff --git a/cxl/monitor.c b/cxl/monitor.c
index 179646562187..0736483cc50a 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 v3 5/6] 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

[ndctl PATCH v3 6/6] ndctl/monitor: use strcmp to compare the reserved word

[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:
$ 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>
---
V3: Improve commit log # Dave
V2: commit log updated # Dave
---
 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 v3 0/6] cxl/monitor and ndctl/monitor fixes

[Zhijian Li (Fujitsu)]

kindly ping

It has been almost a month, and it's doing some bugfix. The progress is a little bit slow anyway :)



On 31/05/2023 10:19, Li Zhijian wrote:
> 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.
> 
> Future, 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.
> 
> Li Zhijian (6):
>    cxl/monitor: Enable default_log and refactor sanity check
>    cxl/monitor: replace monitor.log_file with monitor.ctx.log_file
>    cxl/monitor: use strcmp to compare the reserved word
>    cxl/monitor: always log started message
>    Documentation/cxl/cxl-monitor.txt: Fix inaccurate description
>    ndctl/monitor: use strcmp to compare the reserved word
> 
>   Documentation/cxl/cxl-monitor.txt |  3 +--
>   cxl/monitor.c                     | 45 ++++++++++++++++---------------
>   ndctl/monitor.c                   |  4 +--
>   3 files changed, 26 insertions(+), 26 deletions(-)
> 

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

[Dave Jiang]

On 5/30/23 19:19, 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.
> 
> Here we assigned it a default log per its arguments, and simplify the
> sanity check 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 in
> parse_options_prefix().
> if (strncmp(monitor.log, "./", 2) != 0)
>      fix_filename(prefix, (const char **)&monitor.log);
> 
> Signed-off-by: Li Zhijian <lizhijian@fujitsu.com>
> ---
> V2: exchange order of previous patch1 and patch2 # Alison
>      a few commit log updated
> Signed-off-by: Li Zhijian <lizhijian@fujitsu.com>

Reviewed-by: Dave Jiang <dave.jiang@intel.com>
> ---
>   cxl/monitor.c | 38 ++++++++++++++++++++------------------
>   1 file changed, 20 insertions(+), 18 deletions(-)
> 
> diff --git a/cxl/monitor.c b/cxl/monitor.c
> index e3469b9a4792..c6df2bad3c53 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("standard or relative path for <file> will not work for 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) {

Re: [ndctl PATCH v3 2/6] cxl/monitor: replace monitor.log_file with monitor.ctx.log_file

[Dave Jiang]

On 5/30/23 19:19, 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)
> 
> 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>
> ---
> 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 c6df2bad3c53..f0e3c4c3f45c 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;
>   }

Re: [ndctl PATCH v3 3/6] cxl/monitor: use strcmp to compare the reserved word

[Dave Jiang]

On 5/30/23 19:19, 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.
> 
> Signed-off-by: Li Zhijian <lizhijian@fujitsu.com>

Reviewed-by: Dave Jiang <dave.jiang@intel.com>
> ---
> V3: Improve commit log # Dave
> V2: commit log updated # Dave
> ---
>   cxl/monitor.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/cxl/monitor.c b/cxl/monitor.c
> index f0e3c4c3f45c..179646562187 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+");

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

[Dave Jiang]

On 5/30/23 19:19, Li Zhijian wrote:
> Tell people monitor is starting rather only daemon mode will log this
> message before. It's a minor fix so that whatever stdout or logfile
> is specified, people can see a *normal* message.
> 
> After this patch
>   # cxl monitor
>   cxl monitor started.
>   ^C
>   # cxl monitor -l standard.log
>   ^C
>   # cat standard.log
>   [1684735993.704815571] [818499] cxl monitor started.
>   # cxl monitor --daemon -l /var/log/daemon.log
>   # cat /var/log/daemon.log
>   [1684736075.817150494] [818509] cxl monitor started.
> 
> Signed-off-by: Li Zhijian <lizhijian@fujitsu.com>
Reviewed-by: Dave Jiang <dave.jiang@intel.com>
> ---
> V2: commit log updated # Dave
> ---
>   cxl/monitor.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/cxl/monitor.c b/cxl/monitor.c
> index 179646562187..0736483cc50a 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 v3 6/6] ndctl/monitor: use strcmp to compare the reserved word

[Dave Jiang]

On 5/30/23 19:19, 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:
> $ 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>

Reviewed-by: Dave Jiang <dave.jiang@intel.com>
> ---
> V3: Improve commit log # Dave
> V2: commit log updated # Dave
> ---
>   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 v3 1/6] cxl/monitor: Enable default_log and refactor sanity check

[Verma, Vishal L]

On Wed, 2023-05-31 at 10:19 +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.
> 
> Here we assigned it a default log per its arguments, and simplify the
> sanity check so that it can be consistent with the document.

Avoid using 'we' as it can be ambiguous what / who it it referring to.
Also generally, use an imperative tone for changelogs - e.g. the above
line could just be "Simplify the sanity checks so that the default log
file is assigned correctly, and the behavior is consistent with the
documentation."

> 
> Please note that i also removed following addition stuff, since we have
> added this prefix if needed during parsing the FILENAME in
> parse_options_prefix().

Shouldn't be using "I did xyz.." in a commit message either - change to
imperative, e.g.: "Remove the filename prefix tweaking in
function_foo() since it is unnecessary."

> if (strncmp(monitor.log, "./", 2) != 0)
>     fix_filename(prefix, (const char **)&monitor.log);

Usually no need to include code snippets in changelogs - the details
are easily available in the actual commit itself. Instead describe what
you did and why if it isn't an obvious change.

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

Use the /* comment */ style - ndctl follows the kernel's coding style
whenever applicable.

> +       if (monitor.daemon && monitor.log && !strncmp(monitor.log, "./", 2)) {
> +               error("standard or relative path for <file> will not work for daemon mode\n");

Just to reduce confusion about what 'standard' is and to emphesize that
it's a keyword, maybe reword this:

"relative path or 'standard' are not compatible with daemon mode"

> +               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";

I think the original './standard' was used that way because
fix_filename() added the './' prefix. Note that the keyword used in the
nam page is just 'standard' - so shouldn't this just be using
'standard' rather than './standard'. Similarly, later when you strcmp,
that should also become just 'standard' of course. 

>  
>         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) {

Re: [ndctl PATCH v3 3/6] cxl/monitor: use strcmp to compare the reserved word

[Verma, Vishal L]

On Wed, 2023-05-31 at 10:19 +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.
> 
> Signed-off-by: Li Zhijian <lizhijian@fujitsu.com>
> ---
> V3: Improve commit log # Dave
> V2: commit log updated # Dave
> ---
>  cxl/monitor.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/cxl/monitor.c b/cxl/monitor.c
> index f0e3c4c3f45c..179646562187 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)

As noted in patch 1, this should just be using 'standard', not
'./standard'.

With these patches the behavior becomes:

* if -l standard is used, it creates a file called standard in the cwd
* if -l ./standard is used, it uses stdout

It should behave the opposite way for both of those cases.

>                 monitor.ctx.log_fn = log_standard;
>         else {
>                 monitor.ctx.log_file = fopen(log, "a+");

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

[Verma, Vishal L]

On Wed, 2023-05-31 at 10:19 +0800, Li Zhijian wrote:
> Tell people monitor is starting rather only daemon mode will log this
> message before. It's a minor fix so that whatever stdout or logfile
> is specified, people can see a *normal* message.
> 
> After this patch
>  # cxl monitor
>  cxl monitor started.
>  ^C
>  # cxl monitor -l standard.log
>  ^C
>  # cat standard.log
>  [1684735993.704815571] [818499] cxl monitor started.
>  # cxl monitor --daemon -l /var/log/daemon.log
>  # cat /var/log/daemon.log
>  [1684736075.817150494] [818509] cxl monitor started.
> 
> Signed-off-by: Li Zhijian <lizhijian@fujitsu.com>
> ---
> V2: commit log updated # Dave
> ---
>  cxl/monitor.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/cxl/monitor.c b/cxl/monitor.c
> index 179646562187..0736483cc50a 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");

Actually I don't think this message should go in a log file at all. It
is okay to print this on stderr only in all cases, but if the monitor
log is meant to contain say json, such a message will break that.

Regardless of the format, the fact that the file is present is enough
to see that the monitor was started.

>  
>         rc = monitor_event(ctx);
>  

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

[Verma, Vishal L]

On Tue, 2023-06-27 at 10:17 +0000, Zhijian Li (Fujitsu) wrote:
> kindly ping
> 
> It has been almost a month, and it's doing some bugfix. The progress
> is a little bit slow anyway :)
> 
Hi Li,

Sorry for the delay - the patches are mostly fine with the exception of
a few things I pointed out. Once those are fixed I'll pick these up for
the next release.

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

[Alison Schofield]

On Wed, May 31, 2023 at 10:19:30AM +0800, Li Zhijian wrote:
> 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.
> 
> Li Zhijian (6):
>   cxl/monitor: Enable default_log and refactor sanity check
>   cxl/monitor: replace monitor.log_file with monitor.ctx.log_file
>   cxl/monitor: use strcmp to compare the reserved word
>   cxl/monitor: always log started message
>   Documentation/cxl/cxl-monitor.txt: Fix inaccurate description
>   ndctl/monitor: use strcmp to compare the reserved word

Hi,

Patches 3 & 6 make the same change in 2 different files, with
near identical commit logs. Please consider combining them into
one patch, perhaps something like:

ndctl: use strcmp for reserved word in monitor commands

Thanks,
Alison

> 
>  Documentation/cxl/cxl-monitor.txt |  3 +--
>  cxl/monitor.c                     | 45 ++++++++++++++++---------------
>  ndctl/monitor.c                   |  4 +--
>  3 files changed, 26 insertions(+), 26 deletions(-)
> 
> -- 
> 2.29.2
> 

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

[Zhijian Li (Fujitsu)]

On 06/07/2023 04:53, Verma, Vishal L wrote:
> On Wed, 2023-05-31 at 10:19 +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.
>>
>> Here we assigned it a default log per its arguments, and simplify the
>> sanity check so that it can be consistent with the document.
> 
> Avoid using 'we' as it can be ambiguous what / who it it referring to.
> Also generally, use an imperative tone for changelogs - e.g. the above
> line could just be "Simplify the sanity checks so that the default log
> file is assigned correctly, and the behavior is consistent with the
> documentation."
> 
>>
>> Please note that i also removed following addition stuff, since we have
>> added this prefix if needed during parsing the FILENAME in
>> parse_options_prefix().
> 
> Shouldn't be using "I did xyz.." in a commit message either - change to
> imperative, e.g.: "Remove the filename prefix tweaking in
> function_foo() since it is unnecessary."
> 
>> if (strncmp(monitor.log, "./", 2) != 0)
>>      fix_filename(prefix, (const char **)&monitor.log);
> 
> Usually no need to include code snippets in changelogs - the details
> are easily available in the actual commit itself. Instead describe what
> you did and why if it isn't an obvious change.
> 

Above 3 comments are good to me.


>>
>> Signed-off-by: Li Zhijian <lizhijian@fujitsu.com>
>> ---
>> 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..c6df2bad3c53 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
> 
> Use the /* comment */ style - ndctl follows the kernel's coding style
> whenever applicable.
> 
>> +       if (monitor.daemon && monitor.log && !strncmp(monitor.log, "./", 2)) {
>> +               error("standard or relative path for <file> will not work for daemon mode\n");
> 
> Just to reduce confusion about what 'standard' is and to emphesize that
> it's a keyword, maybe reword this:
> 
> "relative path or 'standard' are not compatible with daemon mode"
> 
>> +               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";
> 
> I think the original './standard' was used that way because
> fix_filename() added the './' prefix. 

At present, './' will still be added by parse_options_prefix() at the beginning.



Note that the keyword used in the
> nam page is just 'standard' - so shouldn't this just be using
> 'standard' rather than './standard'. Similarly, later when you strcmp,
> that should also become just 'standard' of course.

Correspondingly, in order to be compatible with 'standard' input, default log in this case should be './standard' as well.

Thanks
Zhijian

> 
>>   
>>          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) {
> 

Re: [ndctl PATCH v3 3/6] cxl/monitor: use strcmp to compare the reserved word

[Zhijian Li (Fujitsu)]

on 7/6/2023 5:03 AM, Verma, Vishal L wrote:
> On Wed, 2023-05-31 at 10:19 +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.
>>
>> Signed-off-by: Li Zhijian <lizhijian@fujitsu.com>
>> ---
>> V3: Improve commit log # Dave
>> V2: commit log updated # Dave
>> ---
>>   cxl/monitor.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/cxl/monitor.c b/cxl/monitor.c
>> index f0e3c4c3f45c..179646562187 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)
> As noted in patch 1, this should just be using 'standard', not
> './standard'.
>
> With these patches the behavior becomes:
>
> * if -l standard is used, it creates a file called standard in the cwd
> * if -l ./standard is used, it uses stdout

Not really, './standard' will become '././standard'  by 
parse_options_prefix()

Thanks
Zhijian

>
> It should behave the opposite way for both of those cases.
>
>>                  monitor.ctx.log_fn = log_standard;
>>          else {
>>                  monitor.ctx.log_file = fopen(log, "a+");

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

[Zhijian Li (Fujitsu)]

on 7/6/2023 5:16 AM, Verma, Vishal L wrote:
> On Wed, 2023-05-31 at 10:19 +0800, Li Zhijian wrote:
>> Tell people monitor is starting rather only daemon mode will log this
>> message before. It's a minor fix so that whatever stdout or logfile
>> is specified, people can see a *normal* message.
>>
>> After this patch
>>   # cxl monitor
>>   cxl monitor started.
>>   ^C
>>   # cxl monitor -l standard.log
>>   ^C
>>   # cat standard.log
>>   [1684735993.704815571] [818499] cxl monitor started.
>>   # cxl monitor --daemon -l /var/log/daemon.log
>>   # cat /var/log/daemon.log
>>   [1684736075.817150494] [818509] cxl monitor started.
>>
>> Signed-off-by: Li Zhijian <lizhijian@fujitsu.com>
>> ---
>> V2: commit log updated # Dave
>> ---
>>   cxl/monitor.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/cxl/monitor.c b/cxl/monitor.c
>> index 179646562187..0736483cc50a 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");
> Actually I don't think this message should go in a log file at all. It
> is okay to print this on stderr only in all cases, but if the monitor
> log is meant to contain say json, such a message will break that.

wow, you are right. I'm fine to delete the message.
Let's drop this patch so that it's consistent with message of 'ndctl 
monitor'.

Thanks
Zhijian

>
> Regardless of the format, the fact that the file is present is enough
> to see that the monitor was started.
>
>>   
>>          rc = monitor_event(ctx);
>>   

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

[Zhijian Li (Fujitsu)]

on 7/6/2023 7:53 AM, Alison Schofield wrote:
> On Wed, May 31, 2023 at 10:19:30AM +0800, Li Zhijian wrote:
>> 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.
>>
>> Li Zhijian (6):
>>    cxl/monitor: Enable default_log and refactor sanity check
>>    cxl/monitor: replace monitor.log_file with monitor.ctx.log_file
>>    cxl/monitor: use strcmp to compare the reserved word
>>    cxl/monitor: always log started message
>>    Documentation/cxl/cxl-monitor.txt: Fix inaccurate description
>>    ndctl/monitor: use strcmp to compare the reserved word
> Hi,
>
> Patches 3 & 6 make the same change in 2 different files, with
> near identical commit logs. Please consider combining them into
> one patch, perhaps something like:
>
> ndctl: use strcmp for reserved word in monitor commands

Okay, it sounds good to me :)

Thanks
Zhijian

>
> Thanks,
> Alison
>
>>   Documentation/cxl/cxl-monitor.txt |  3 +--
>>   cxl/monitor.c                     | 45 ++++++++++++++++---------------
>>   ndctl/monitor.c                   |  4 +--
>>   3 files changed, 26 insertions(+), 26 deletions(-)
>>
>> -- 
>> 2.29.2
>>

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

[Verma, Vishal L]

On Mon, 2023-07-10 at 10:53 +0000, Zhijian Li (Fujitsu) wrote:
> > > 
> > >          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";
> > 
> > I think the original './standard' was used that way because
> > fix_filename() added the './' prefix. 
> 
> At present, './' will still be added by parse_options_prefix() at the
> beginning.
> 
Ah yes I missed this - I must've confused my test attempts - you're
right, 'standard' behaves as expected (stdout) and './standard also
works as expected (creates a file called standard in the cwd).