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 <lizhij...@fujitsu.com>
>> ---
>> V2: exchange order of previous patch1 and patch2 # Alison
>> a few commit log updated
>> Signed-off-by: Li Zhijian <lizhij...@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) {
>
