lkml.org 
[lkml]   [2021]   [Dec]   [9]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    Patch in this message
    /
    Date
    From
    SubjectRe: [PATCH v7 13/13] user_events: Use __get_rel_str for relative string fields
    Hi Beau,

    On Thu, 9 Dec 2021 14:32:10 -0800
    Beau Belgrave <beaub@linux.microsoft.com> wrote:

    > Switch between __get_str and __get_rel_str within the print_fmt of
    > user_events. Add unit test to ensure print_fmt is correct on known
    > types.
    >
    > Signed-off-by: Beau Belgrave <beaub@linux.microsoft.com>
    > ---
    > kernel/trace/trace_events_user.c | 24 ++-
    > .../selftests/user_events/ftrace_test.c | 166 ++++++++++++++++++
    > 2 files changed, 182 insertions(+), 8 deletions(-)
    >
    > diff --git a/kernel/trace/trace_events_user.c b/kernel/trace/trace_events_user.c
    > index 56eb58ddb4cf..3779fa2ca14a 100644
    > --- a/kernel/trace/trace_events_user.c
    > +++ b/kernel/trace/trace_events_user.c
    > @@ -257,7 +257,7 @@ static int user_event_add_field(struct user_event *user, const char *type,
    > goto add_field;
    >
    > add_validator:
    > - if (strstr(type, "char[") != 0)
    > + if (strstr(type, "char") != 0)
    > validator_flags |= VALIDATOR_ENSURE_NULL;

    What is this change for? This seems not related to the other changes.
    (Also, what happen if it is a single char type?)

    >
    > validator = kmalloc(sizeof(*validator), GFP_KERNEL);
    > @@ -456,14 +456,21 @@ static const char *user_field_format(const char *type)
    > return "%llu";
    > }
    >
    > -static bool user_field_is_dyn_string(const char *type)
    > +static bool user_field_is_dyn_string(const char *type, const char **str_func)
    > {
    > - if (str_has_prefix(type, "__data_loc ") ||
    > - str_has_prefix(type, "__rel_loc "))
    > - if (strstr(type, "char[") != 0)
    > - return true;
    > + if (str_has_prefix(type, "__data_loc ")) {
    > + *str_func = "__get_str";
    > + goto check;
    > + }
    > +
    > + if (str_has_prefix(type, "__rel_loc ")) {
    > + *str_func = "__get_rel_str";
    > + goto check;
    > + }
    >
    > return false;
    > +check:
    > + return strstr(type, "char") != 0;
    > }
    >
    > #define LEN_OR_ZERO (len ? len - pos : 0)
    > @@ -472,6 +479,7 @@ static int user_event_set_print_fmt(struct user_event *user, char *buf, int len)
    > struct ftrace_event_field *field, *next;
    > struct list_head *head = &user->fields;
    > int pos = 0, depth = 0;
    > + const char *str_func;
    >
    > pos += snprintf(buf + pos, LEN_OR_ZERO, "\"");
    >
    > @@ -488,9 +496,9 @@ static int user_event_set_print_fmt(struct user_event *user, char *buf, int len)
    > pos += snprintf(buf + pos, LEN_OR_ZERO, "\"");
    >
    > list_for_each_entry_safe_reverse(field, next, head, link) {
    > - if (user_field_is_dyn_string(field->type))
    > + if (user_field_is_dyn_string(field->type, &str_func))
    > pos += snprintf(buf + pos, LEN_OR_ZERO,
    > - ", __get_str(%s)", field->name);
    > + ", %s(%s)", str_func, field->name);
    > else
    > pos += snprintf(buf + pos, LEN_OR_ZERO,
    > ", REC->%s", field->name);
    > diff --git a/tools/testing/selftests/user_events/ftrace_test.c b/tools/testing/selftests/user_events/ftrace_test.c

    Just a nitpick, if possible, please split this part from the kernel update.

    > index 16aff1fb295a..b2e5c0765a68 100644
    > --- a/tools/testing/selftests/user_events/ftrace_test.c
    > +++ b/tools/testing/selftests/user_events/ftrace_test.c
    > @@ -20,6 +20,7 @@ const char *data_file = "/sys/kernel/debug/tracing/user_events_data";
    > const char *status_file = "/sys/kernel/debug/tracing/user_events_status";
    > const char *enable_file = "/sys/kernel/debug/tracing/events/user_events/__test_event/enable";
    > const char *trace_file = "/sys/kernel/debug/tracing/trace";
    > +const char *fmt_file = "/sys/kernel/debug/tracing/events/user_events/__test_event/format";
    >
    > static int trace_bytes(void)
    > {
    > @@ -47,6 +48,61 @@ static int trace_bytes(void)
    > return bytes;
    > }
    >
    > +static int get_print_fmt(char *buffer, int len)
    > +{
    > + FILE *fp = fopen(fmt_file, "r");
    > + int c, index = 0, last = 0;
    > +
    > + if (!fp)
    > + return -1;
    > +
    > + /* Read until empty line (Skip Common) */
    > + while (true) {
    > + c = getc(fp);
    > +
    > + if (c == EOF)
    > + break;
    > +
    > + if (last == '\n' && c == '\n')
    > + break;
    > +
    > + last = c;
    > + }

    Another nitpick, maybe you need a function like skip_until_empty_line(fp)
    and repeat it like this.

    if (skip_until_empty_line(fp) < 0)
    goto out;
    if (skip_until_empty_line(fp) < 0)
    goto out;

    > +
    > + last = 0;
    > +
    > + /* Read until empty line (Skip Properties) */
    > + while (true) {
    > + c = getc(fp);
    > +
    > + if (c == EOF)
    > + break;
    > +
    > + if (last == '\n' && c == '\n')
    > + break;
    > +
    > + last = c;
    > + }
    > +
    > + /* Read in print_fmt: */
    > + while (len > 1) {
    > + c = getc(fp);
    > +
    > + if (c == EOF || c == '\n')
    > + break;
    > +
    > + buffer[index++] = c;
    > +
    > + len--;
    > + }

    And here you can use fgets(buffer, len, fp).


    Thank you,

    > +
    > + buffer[index] = 0;
    > +
    > + fclose(fp);
    > +
    > + return 0;
    > +}
    > +
    > static int clear(void)
    > {
    > int fd = open(data_file, O_RDWR);
    > @@ -63,6 +119,44 @@ static int clear(void)
    > return 0;
    > }
    >
    > +static int check_print_fmt(const char *event, const char *expected)
    > +{
    > + struct user_reg reg = {0};
    > + char print_fmt[256];
    > + int ret;
    > + int fd;
    > +
    > + /* Ensure cleared */
    > + ret = clear();
    > +
    > + if (ret != 0)
    > + return ret;
    > +
    > + fd = open(data_file, O_RDWR);
    > +
    > + if (fd == -1)
    > + return fd;
    > +
    > + reg.size = sizeof(reg);
    > + reg.name_args = (__u64)event;
    > +
    > + /* Register should work */
    > + ret = ioctl(fd, DIAG_IOCSREG, &reg);
    > +
    > + close(fd);
    > +
    > + if (ret != 0)
    > + return ret;
    > +
    > + /* Ensure correct print_fmt */
    > + ret = get_print_fmt(print_fmt, sizeof(print_fmt));
    > +
    > + if (ret != 0)
    > + return ret;
    > +
    > + return strcmp(print_fmt, expected);
    > +}
    > +
    > FIXTURE(user) {
    > int status_fd;
    > int data_fd;
    > @@ -282,6 +376,78 @@ TEST_F(user, write_validator) {
    > ASSERT_EQ(EFAULT, errno);
    > }
    >
    > +TEST_F(user, print_fmt) {
    > + int ret;
    > +
    > + ret = check_print_fmt("__test_event __rel_loc char[] data",
    > + "print fmt: \"data=%s\", __get_rel_str(data)");
    > + ASSERT_EQ(0, ret);
    > +
    > + ret = check_print_fmt("__test_event __data_loc char[] data",
    > + "print fmt: \"data=%s\", __get_str(data)");
    > + ASSERT_EQ(0, ret);
    > +
    > + ret = check_print_fmt("__test_event s64 data",
    > + "print fmt: \"data=%lld\", REC->data");
    > + ASSERT_EQ(0, ret);
    > +
    > + ret = check_print_fmt("__test_event u64 data",
    > + "print fmt: \"data=%llu\", REC->data");
    > + ASSERT_EQ(0, ret);
    > +
    > + ret = check_print_fmt("__test_event s32 data",
    > + "print fmt: \"data=%d\", REC->data");
    > + ASSERT_EQ(0, ret);
    > +
    > + ret = check_print_fmt("__test_event u32 data",
    > + "print fmt: \"data=%u\", REC->data");
    > + ASSERT_EQ(0, ret);
    > +
    > + ret = check_print_fmt("__test_event int data",
    > + "print fmt: \"data=%d\", REC->data");
    > + ASSERT_EQ(0, ret);
    > +
    > + ret = check_print_fmt("__test_event unsigned int data",
    > + "print fmt: \"data=%u\", REC->data");
    > + ASSERT_EQ(0, ret);
    > +
    > + ret = check_print_fmt("__test_event s16 data",
    > + "print fmt: \"data=%d\", REC->data");
    > + ASSERT_EQ(0, ret);
    > +
    > + ret = check_print_fmt("__test_event u16 data",
    > + "print fmt: \"data=%u\", REC->data");
    > + ASSERT_EQ(0, ret);
    > +
    > + ret = check_print_fmt("__test_event short data",
    > + "print fmt: \"data=%d\", REC->data");
    > + ASSERT_EQ(0, ret);
    > +
    > + ret = check_print_fmt("__test_event unsigned short data",
    > + "print fmt: \"data=%u\", REC->data");
    > + ASSERT_EQ(0, ret);
    > +
    > + ret = check_print_fmt("__test_event s8 data",
    > + "print fmt: \"data=%d\", REC->data");
    > + ASSERT_EQ(0, ret);
    > +
    > + ret = check_print_fmt("__test_event u8 data",
    > + "print fmt: \"data=%u\", REC->data");
    > + ASSERT_EQ(0, ret);
    > +
    > + ret = check_print_fmt("__test_event char data",
    > + "print fmt: \"data=%d\", REC->data");
    > + ASSERT_EQ(0, ret);
    > +
    > + ret = check_print_fmt("__test_event unsigned char data",
    > + "print fmt: \"data=%u\", REC->data");
    > + ASSERT_EQ(0, ret);
    > +
    > + ret = check_print_fmt("__test_event char[4] data",
    > + "print fmt: \"data=%s\", REC->data");
    > + ASSERT_EQ(0, ret);
    > +}
    > +
    > int main(int argc, char **argv)
    > {
    > return test_harness_run(argc, argv);
    > --
    > 2.17.1
    >


    --
    Masami Hiramatsu <mhiramat@kernel.org>

    \
     
     \ /
      Last update: 2021-12-10 02:24    [W:3.764 / U:0.040 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site