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

["Zhijian Li (Fujitsu)" <lizhijian@fujitsu.com>]

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