From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by smtp.lore.kernel.org (Postfix) with ESMTP id EFEE0C433EF for ; Fri, 10 Dec 2021 01:23:35 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S235356AbhLJB1I (ORCPT ); Thu, 9 Dec 2021 20:27:08 -0500 Received: from sin.source.kernel.org ([145.40.73.55]:51842 "EHLO sin.source.kernel.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S230387AbhLJB1H (ORCPT ); Thu, 9 Dec 2021 20:27:07 -0500 Received: from smtp.kernel.org (relay.kernel.org [52.25.139.140]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by sin.source.kernel.org (Postfix) with ESMTPS id 47D62CE25DC; Fri, 10 Dec 2021 01:23:32 +0000 (UTC) Received: by smtp.kernel.org (Postfix) with ESMTPSA id 4D20EC004DD; Fri, 10 Dec 2021 01:23:29 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1639099410; bh=m+LAvWo3dIWq/3odGCreFX5XeaF+M662syX5Hp3Tc2g=; h=Date:From:To:Cc:Subject:In-Reply-To:References:From; b=H3VQ0GKd4dlbw972MPPRxFmMtzP5DDy2sUHrm7FQuH+MQkvz85MH7NIpomC2GtrXl NwWWfd7GzQywQTR+wPMeLk5hqiaXkLKVxrnNhT/CobHS5Y8Cubyohn8zAzo2dYPU9T 64ZXOM7itYzOq27Yxp7zVhqNkoKSqgdVcUTPvi9gORLLVDhk5nRziEy/Lu8eumutrP frChZhLXpeCVGHrHN4z6tlk51noRSXIztJQzeozMGu32+mqHSOd9YxZM1Y5eOSHMRH 1hLTTZdLJDFC0UA91/D2ar1rGJMQFBzosmnTNFpxvsKieF69BE9zJVIm65Ia+h8GgX ir6oGZTgrt2zA== Date: Fri, 10 Dec 2021 10:23:27 +0900 From: Masami Hiramatsu To: Beau Belgrave Cc: rostedt@goodmis.org, linux-trace-devel@vger.kernel.org, linux-kernel@vger.kernel.org Subject: Re: [PATCH v7 13/13] user_events: Use __get_rel_str for relative string fields Message-Id: <20211210102327.ab971d529613271ab1bf0073@kernel.org> In-Reply-To: <20211209223210.1818-14-beaub@linux.microsoft.com> References: <20211209223210.1818-1-beaub@linux.microsoft.com> <20211209223210.1818-14-beaub@linux.microsoft.com> X-Mailer: Sylpheed 3.7.0 (GTK+ 2.24.32; x86_64-pc-linux-gnu) Mime-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Hi Beau, On Thu, 9 Dec 2021 14:32:10 -0800 Beau Belgrave 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 > --- > 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, ®); > + > + 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