Re: [PATCH v4 07/10] cxl: add monitor command to cxl

["Verma, Vishal L" <vishal.l.verma@intel.com>]

On Tue, 2022-11-08 at 11:30 -0700, Dave Jiang wrote:
> Connect the monitoring functionality to the cxl monitor command. Add basic
> functionality to the cxl monitor command where it can be launched as a daemon
> and logging can be designated to stdout or a file.
> 
> Signed-off-by: Dave Jiang <dave.jiang@intel.com>
> ---
>  cxl/builtin.h |    1 +
>  cxl/cxl.c     |    1 +
>  cxl/monitor.c |   75 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 77 insertions(+)

Feels likee this patch should be squashed with patch 5. Unless there's
an underlying reason for the split I'm missing, the standalone addition
of the monitor_event() function doesn't do anything, and doesn't feel
like a clean break even for reviewability.

> 
> diff --git a/cxl/builtin.h b/cxl/builtin.h
> index b28c2213993b..34c5cfb49051 100644
> --- a/cxl/builtin.h
> +++ b/cxl/builtin.h
> @@ -22,4 +22,5 @@ int cmd_create_region(int argc, const char **argv, struct cxl_ctx *ctx);
>  int cmd_enable_region(int argc, const char **argv, struct cxl_ctx *ctx);
>  int cmd_disable_region(int argc, const char **argv, struct cxl_ctx *ctx);
>  int cmd_destroy_region(int argc, const char **argv, struct cxl_ctx *ctx);
> +int cmd_monitor(int argc, const char **argv, struct cxl_ctx *ctx);
>  #endif /* _CXL_BUILTIN_H_ */
> diff --git a/cxl/cxl.c b/cxl/cxl.c
> index dd1be7a054a1..3be7026f43d3 100644
> --- a/cxl/cxl.c
> +++ b/cxl/cxl.c
> @@ -76,6 +76,7 @@ static struct cmd_struct commands[] = {
>         { "enable-region", .c_fn = cmd_enable_region },
>         { "disable-region", .c_fn = cmd_disable_region },
>         { "destroy-region", .c_fn = cmd_destroy_region },
> +       { "monitor", .c_fn = cmd_monitor },
>  };
>  
>  int main(int argc, const char **argv)
> diff --git a/cxl/monitor.c b/cxl/monitor.c
> index 518d8142e40d..d8e498392ed7 100644
> --- a/cxl/monitor.c
> +++ b/cxl/monitor.c
> @@ -32,11 +32,15 @@
>  #include "event_trace.h"
>  
>  static const char *cxl_system = "cxl";
> +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;
>  } monitor;
>  
>  static void log_standard(struct log_ctx *ctx, int priority, const char *file,
> @@ -163,3 +167,74 @@ epoll_err:
>         free(events);
>         return rc;
>  }
> +
> +int cmd_monitor(int argc, const char **argv, struct cxl_ctx *ctx)
> +{
> +       const struct option options[] = {
> +               OPT_FILENAME('l', "log", &monitor.log,
> +                               "<file> | standard",
> +                               "where to output the monitor's notification"),
> +               OPT_BOOLEAN('\0', "daemon", &monitor.daemon,
> +                               "run cxl monitor as a daemon"),
> +               OPT_BOOLEAN('u', "human", &monitor.human,
> +                               "use human friendly output formats"),
> +               OPT_BOOLEAN('v', "verbose", &monitor.verbose,
> +                               "emit extra debug messages to log"),
> +               OPT_END(),
> +       };
> +       const char * const u[] = {
> +               "cxl monitor [<options>]",
> +               NULL
> +       };
> +       const char *prefix ="./";
> +       int rc = 0, i;
> +
> +       argc = parse_options_prefix(argc, argv, prefix, options, u, 0);
> +       for (i = 0; i < argc; i++)
> +               error("unknown parameter \"%s\"\n", argv[i]);
> +       if (argc)
> +               usage_with_options(u, options);
> +
> +       log_init(&monitor.ctx, "cxl/monitor", "CXL_MONITOR_LOG");
> +       monitor.ctx.log_fn = 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 (monitor.daemon) {
> +               if (daemon(0, 0) != 0) {
> +                       err(&monitor, "daemon start failed\n");
> +                       goto out;
> +               }
> +               info(&monitor, "cxl monitor daemon started.\n");
> +       }
> +
> +       rc = monitor_event(ctx);
> +
> +out:
> +       if (monitor.log_file)
> +               fclose(monitor.log_file);
> +       return rc;
> +}
> 
>