[PATCH 15/18] cmd: log: Add commands to manipulate filters
Sean Anderson
seanga2 at gmail.com
Tue Oct 6 23:51:45 CEST 2020
On 10/6/20 5:14 PM, Heinrich Schuchardt wrote:
> On 10/6/20 9:16 PM, Sean Anderson wrote:
>> This adds several commands to add, list, and remove log filters. Due to the
>> complexity of adding a filter, `log filter-list` uses options instead of
>> positional arguments.
>>
>> These commands have been added as subcommands to log by using a dash to
>> join the subcommand and subsubcommand. This is stylistic, and they could be
>> converted to proper subsubcommands if it is wished.
>>
>> Signed-off-by: Sean Anderson <seanga2 at gmail.com>
>> ---
>>
>> cmd/Kconfig | 1 +
>> cmd/log.c | 213 ++++++++++++++++++++++++++++++++++++++++++++++++++++
>> 2 files changed, 214 insertions(+)
>>
>> diff --git a/cmd/Kconfig b/cmd/Kconfig
>> index 999b6cf239..c5cb112171 100644
>> --- a/cmd/Kconfig
>> +++ b/cmd/Kconfig
>> @@ -2255,6 +2255,7 @@ config CMD_KGDB
>> config CMD_LOG
>> bool "log - Generation, control and access to logging"
>> select LOG
>> + select GETOPT
>> help
>> This provides access to logging features. It allows the output of
>> log data to be controlled to a limited extent (setting up the default
>> diff --git a/cmd/log.c b/cmd/log.c
>> index 71bfdbbc0f..c9eaff7b94 100644
>> --- a/cmd/log.c
>> +++ b/cmd/log.c
>> @@ -7,6 +7,7 @@
>> #include <common.h>
>> #include <command.h>
>> #include <dm.h>
>> +#include <getopt.h>
>> #include <log.h>
>>
>> static char log_fmt_chars[LOGF_COUNT] = "clFLfm";
>> @@ -43,6 +44,195 @@ static int do_log_level(struct cmd_tbl *cmdtp, int flag, int argc,
>> return CMD_RET_SUCCESS;
>> }
>>
>> +static int do_log_filter_list(struct cmd_tbl *cmdtp, int flag, int argc,
>> + char *const argv[])
>> +{
>> + int opt;
>> + const char *drv_name = "console";
>> + struct getopt_state gs;
>> + struct log_filter *filt;
>> + struct log_device *ldev;
>> +
>> + getopt_init_state(&gs);
>> + while ((opt = getopt(&gs, argc, argv, "d:")) > 0) {
>> + switch (opt) {
>> + case 'd':
>> + drv_name = gs.optarg;
>> + break;
>> + default:
>> + return CMD_RET_USAGE;
>> + }
>> + }
>> +
>> + if (gs.optind != argc)
>> + return CMD_RET_USAGE;
>> +
>> + ldev = log_device_find_by_name(drv_name);
>> + if (!ldev) {
>> + printf("Could not find log device for \"%s\"\n", drv_name);
>> + return CMD_RET_FAILURE;
>> + }
>> +
>> + printf("num policy level categories files\n");
>> + list_for_each_entry(filt, &ldev->filter_head, sibling_node) {
>> + printf("%3d %6.6s %s%-7.7s ", filt->filter_num,
>> + filt->flags & LOGFF_DENY ? "deny" : "allow",
>> + filt->flags & LOGFF_LEVEL_MIN ? ">=" : "<=",
>> + log_get_level_name(filt->level));
>> +
>> + if (filt->flags & LOGFF_HAS_CAT) {
>> + int i;
>> +
>> + if (filt->cat_list[0] != LOGC_END)
>> + printf("%16.16s %s\n",
>> + log_get_cat_name(filt->cat_list[0]),
>> + filt->file_list ? filt->file_list : "");
>> +
>> + for (i = 1; i < LOGF_MAX_CATEGORIES &&
>> + filt->cat_list[i] != LOGC_END; i++)
>> + printf("%20c %16.16s\n", ' ',
>> + log_get_cat_name(filt->cat_list[i]));
>> + } else {
>> + printf("%16c %s\n", ' ',
>> + filt->file_list ? filt->file_list : "");
>> + }
>> + }
>> +
>> + return CMD_RET_SUCCESS;
>> +}
>> +
>> +static int do_log_filter_add(struct cmd_tbl *cmdtp, int flag, int argc,
>> + char *const argv[])
>> +{
>> + bool level_set = false;
>> + bool print_num = false;
>> + bool type_set = false;
>> + char *file_list = NULL;
>> + const char *drv_name = "console";
>> + int opt, err;
>> + int cat_count = 0;
>> + int flags = 0;
>> + enum log_category_t cat_list[LOGF_MAX_CATEGORIES + 1];
>> + enum log_level_t level = LOGL_MAX;
>> + struct getopt_state gs;
>> +
>> + getopt_init_state(&gs);
>> + while ((opt = getopt(&gs, argc, argv, "Ac:d:Df:l:L:p")) > 0) {
>> + switch (opt) {
>> + case 'A':
>> +#define do_type() do { \
>> + if (type_set) { \
>> + printf("Allow or deny set twice\n"); \
>> + return CMD_RET_USAGE; \
>> + } \
>> + type_set = true; \
>> +} while (0)
>> + do_type();
>> + break;
>> + case 'c': {
>> + enum log_category_t cat;
>> +
>> + if (cat_count >= LOGF_MAX_CATEGORIES) {
>> + printf("Too many categories\n");
>> + return CMD_RET_FAILURE;
>> + }
>> +
>> + cat = log_get_cat_by_name(gs.optarg);
>> + if (cat == LOGC_NONE) {
>> + printf("Unknown category \"%s\"\n", gs.optarg);
>> + return CMD_RET_FAILURE;
>> + }
>> +
>> + cat_list[cat_count++] = cat;
>> + break;
>> + }
>> + case 'd':
>> + drv_name = gs.optarg;
>> + break;
>> + case 'D':
>> + do_type();
>> + flags |= LOGFF_DENY;
>> + break;
>> + case 'f':
>> + file_list = gs.optarg;
>> + break;
>> + case 'l':
>> +#define do_level() do { \
>> + if (level_set) { \
>> + printf("Log level set twice\n"); \
>> + return CMD_RET_USAGE; \
>> + } \
>> + level = parse_log_level(gs.optarg); \
>> + if (level == LOGL_NONE) \
>> + return CMD_RET_FAILURE; \
>> + level_set = true; \
>> +} while (0)
>> + do_level();
>> + break;
>> + case 'L':
>> + do_level();
>> + flags |= LOGFF_LEVEL_MIN;
>> + break;
>> + case 'p':
>> + print_num = true;
>> + break;
>> + default:
>> + return CMD_RET_USAGE;
>> + }
>> + }
>> +
>> + if (gs.optind != argc)
>> + return CMD_RET_USAGE;
>> +
>> + cat_list[cat_count] = LOGC_END;
>> + err = log_add_filter_flags(drv_name, cat_count ? cat_list : NULL, level,
>> + file_list, flags);
>> + if (err < 0) {
>> + printf("Could not add filter (err = %d)\n", err);
>> + return CMD_RET_FAILURE;
>> + } else if (print_num) {
>> + printf("%d\n", err);
>> + }
>> +
>> + return CMD_RET_SUCCESS;
>> +}
>> +
>> +static int do_log_filter_remove(struct cmd_tbl *cmdtp, int flag, int argc,
>> + char *const argv[])
>> +{
>> + int opt, err;
>> + ulong filter_num;
>> + const char *drv_name = "console";
>> + struct getopt_state gs;
>> +
>> + getopt_init_state(&gs);
>> + while ((opt = getopt(&gs, argc, argv, "d:")) > 0) {
>> + switch (opt) {
>> + case 'd':
>> + drv_name = gs.optarg;
>> + break;
>> + default:
>> + return CMD_RET_USAGE;
>> + }
>> + }
>> +
>> + if (gs.optind + 1 != argc)
>> + return CMD_RET_USAGE;
>> +
>> + if (strict_strtoul(argv[gs.optind], 10, &filter_num)) {
>> + printf("Invalid filter number \"%s\"\n", argv[gs.optind]);
>> + return CMD_RET_FAILURE;
>> + }
>> +
>> + err = log_remove_filter(drv_name, filter_num);
>> + if (err) {
>> + printf("Could not remove filter (err = %d)\n", err);
>> + return CMD_RET_FAILURE;
>> + }
>> +
>> + return CMD_RET_SUCCESS;
>> +}
>> +
>> static int do_log_format(struct cmd_tbl *cmdtp, int flag, int argc,
>> char *const argv[])
>> {
>> @@ -122,6 +312,25 @@ static char log_help_text[] =
>> #if CONFIG_IS_ENABLED(LOG_TEST)
>> "log test - run log tests\n"
>
> This is probably what a typical user is least interested in. Please,
> move it to the end of the description.
Ok.
>
>> #endif
>> + "log filter-list [OPTIONS] - list all filters for a log driver\n"
>> + "\t-d <driver> - Specify the log driver to add the filter to; defaults\n"> + "\t to console\n"
>
> The text seems not to relate to filter-list.
Oh, whoops, the text should be something like
>> + "\t-d <driver> - Specify the log driver to list filters from; defaults to console\n"
I originally wrote that line for the log filter-add command.
> Why don't we filter all log drivers by default? When would a user be
> interested to filter by driver?
Good question. This part of the API seems to have no real users at the
moment. One use could be in restricting the log level over ethernet. For
example, you could have the default log level on the system be INFO, but
deny messages above WARNING from the syslog driver. It might be easier
to do this via a driver-specific log level instead. However, I think we
should leave per-driver filters in until it becomes clear that they are
not necessary.
>
> How should I know which drivers and categories exist on my system?
>
>> + "log filter-add [OPTIONS] - add a new filter to a driver\n"
>> + "\t-A - Allow messages matching this filter; mutually exclusive with -D\n"
>> + "\t This is the default.\n"
>> + "\t-c <category> - Category to match; may be specified multiple times\n"
>> + "\t-d <driver> - Specify the log driver to add the filter to; defaults\n"
>> + "\t to console\n"
>> + "\t-D - Deny messages matching this filter; mutually exclusive with -A\n"
>> + "\t-f <files_list> - A comma-separated list of files to match\n"
>> + "\t-l <level> - Match log levels below this level; mutually exclusive\n"
>> + "\t with -L\n"
>
> Is this below or below and equal?
Below and equal. Perhaps something like
>> + "\t-l <level> - Match log levels less than or equal to <level>; mutually exclusive\n"
would be better.
>
>> + "\t-L <level> - Match log levels above this level; mutually exclusive\n"
>> + "\t with -l\n"
>
> Is this above or above and equal?
This is above and equal.
There is a bit of asymmetry here, as both of the comparison operators
include the listed level. I'm not sure what the most ergonomic choice
here is.
>
>> + "\t-p - Print the filter number on success\n"
>
> Why do we need this parameter? Can't we simply always print the filter
> numbers or never?
Commands should be silent if they succeed, especially simple ones like
this. For a human, it is always possible to find the filter number of a
given filter by examining the output of `log filter-list`. However, it
is useful for machines (e.g. for testing) if there is an option to get
the filter number when it is produced.
>> + "log filter-remove [OPTIONS] <num> - Remove filter number <num>\n"
>> + "\t-d <driver> - Specify the log driver to add the filter to; defaults\n"
>> + "\t to console\n"
>
> This entry seems not to be related to removal.
Yes, same mistake as above.
>
> How do I simply delete all filters?
You must delete them one-by-one. This could be added.
> How do I remove all filters for a category?
You must delete them one-by-one. This could also be added.
However, I would like to keep the number of features to a minimum until
it becomes clear how these filters will be used. I think a remove-all
version would probably be useful, but I'm less convinced about removing
filters for a category. Should there also be a command to remove all
filters for a given file?
--Sean
More information about the U-Boot
mailing list