| Message ID | 20230513142038.753351-4-lizhijian@fujitsu.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | cxl/monitor and ndctl/monitor fixes | expand |
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 > >
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 >> >>
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; } }
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(-)