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