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 X-Spam-Level: X-Spam-Status: No, score=-0.6 required=3.0 tests=DKIM_SIGNED,DKIM_VALID, DKIM_VALID_AU,FREEMAIL_FORGED_FROMDOMAIN,FREEMAIL_FROM, HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS, URIBL_BLOCKED autolearn=no autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 8474DC2D0FC for ; Wed, 13 May 2020 03:56:38 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id 5C8B9206F5 for ; Wed, 13 May 2020 03:56:38 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (2048-bit key) header.d=gmail.com header.i=@gmail.com header.b="sltm8weU" Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1727902AbgEMD4i (ORCPT ); Tue, 12 May 2020 23:56:38 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:50796 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-FAIL-OK-FAIL) by vger.kernel.org with ESMTP id S1727107AbgEMD4h (ORCPT ); Tue, 12 May 2020 23:56:37 -0400 Received: from mail-pg1-x542.google.com (mail-pg1-x542.google.com [IPv6:2607:f8b0:4864:20::542]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 86AB6C061A0C for ; Tue, 12 May 2020 20:56:36 -0700 (PDT) Received: by mail-pg1-x542.google.com with SMTP id f23so6513151pgj.4 for ; Tue, 12 May 2020 20:56:36 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=vgS23tQubYZrhPMXC3gzH5oCP0nEO8ZDQuMOyYO0pcg=; b=sltm8weUsDXI/BDjn9RcpH9CPHcVz94AvoDSSEXKa4p53+jIW750coOt/LZocZQCb5 +4rkGTJwQnROEe8PKWVXPcByPfnKvw5FXv7JqtMftb3ib8cR3oHBNACOZGL9U8uyusky 6duN/wUu7aGCE698aU9KWvWMSWu8vU9haBOXGzUO5r9b4AaTIALcyJofXED7yCWmcbDp 0CtqLNxPtaEGz2gP/O+kyOiEZNfztZ11N6XlgUBi+wB+eLVJ6vp7s1iq0Gt6982yJiTI otmBSZHo5E6gZO4VKfAr6KPv/oFIae2xt0Eh1MmlRTrHgNlFCCMVKhoYHbZWR+znycc/ bkTQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=vgS23tQubYZrhPMXC3gzH5oCP0nEO8ZDQuMOyYO0pcg=; b=hAmipBogcFOyZjpmXpbmLMhbRo6bqberYVqeaw0BoA1/UjDfiJ1jArs1ouz6kKCFfz dVAhEacXA5txPTc2XjzAjPkCxd8RpwgeKCgNp1j0x881xiIfyf0NKnaMSouXVRBmQnc9 70BOliQY3WP2RrZS4tVBGQWteD5wKFg9175VFC6w0hzIG5RdZUwJkGTkmy2XLAm77wUF +kng6ucoFesxJ4CIcgtURv38OSvkt8AO7Gxb8cw1zaJNY5YNGwfIz/X8qQdPsJTCjmrp o8fyWLDRiR9YqEL4THM2hTHu6gxuR217DMm6d6MXEvtju9DY42YjieVqZN0CZHnv7pfn QE1Q== X-Gm-Message-State: AGi0PuYqq4boJzLh/vJ7L8W9157XTTejxUX5b18ALuSOFmKF37vD3WjX lGxv/Xc93JbBf01StDLcAnY9pRJIivIiTMyGNx7JZd72 X-Google-Smtp-Source: APiQypKVLNWBN4GBde/RhqM9P8BkSsqf6urfHSJLrttfn2dI05q/xEnrrUe5n48f6KOA31hC0AWBwr5EVi1lwe/pX+Y= X-Received: by 2002:a63:175c:: with SMTP id 28mr21090643pgx.44.1589342195788; Tue, 12 May 2020 20:56:35 -0700 (PDT) MIME-Version: 1.0 References: <20200512155127.2310670-1-tz.stoyanov@gmail.com> <20200512211409.63fb63c5@oasis.local.home> In-Reply-To: <20200512211409.63fb63c5@oasis.local.home> From: Tzvetomir Stoyanov Date: Wed, 13 May 2020 06:56:24 +0300 Message-ID: Subject: Re: [PATCH v2] trace-cmd: Add support for more printk format specifiers To: Steven Rostedt Cc: linux-trace-devel@vger.kernel.org, johannes@sipsolutions.net Content-Type: text/plain; charset="UTF-8" Sender: linux-trace-devel-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-trace-devel@vger.kernel.org On Wed, May 13, 2020 at 4:14 AM Steven Rostedt wrote: > > On Tue, 12 May 2020 18:51:27 +0300 > "Tzvetomir Stoyanov (VMware)" wrote: > > > -static void print_ip4_addr(struct trace_seq *s, char i, unsigned char *buf) > > +static int parse_ip4_print_args(struct tep_handle *tep, > > + const char *ptr, bool *reverse) > > +{ > > + int ret = 0; > > + > > + *reverse = false; > > + > > + /* hnbl */ > > + switch (*ptr) { > > + case 'h': > > + if (tep->file_bigendian) > > + *reverse = false; > > + else > > + *reverse = true; > > + ret++; > > + break; > > + case 'l': > > + *reverse = true; > > + ret++; > > + break; > > + case 'n': > > + case 'b': > > + ret++; > > + /* fall through */ > > + default: > > + *reverse = false; > > + break; > > + } > > + > > + return ret; > > +} > > +static void print_ip4_addr(struct trace_seq *s, char i, bool reverse, unsigned char *buf) > > Need a space between the end bracket and the next function. > > > { > > const char *fmt; > > > > @@ -4555,7 +4604,11 @@ static void print_ip4_addr(struct trace_seq *s, char i, unsigned char *buf) > > else > > fmt = "%d.%d.%d.%d"; > > > > - trace_seq_printf(s, fmt, buf[0], buf[1], buf[2], buf[3]); > > + if (reverse) > > + trace_seq_printf(s, fmt, buf[3], buf[2], buf[1], buf[0]); > > + else > > + trace_seq_printf(s, fmt, buf[0], buf[1], buf[2], buf[3]); > > + > > } > > > > > > +int guid_index[16] = {3, 2, 1, 0, 5, 4, 7, 6, 8, 9, 10, 11, 12, 13, 14, 15}; > > +int uuid_index[16] = {0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15}; > > +static int print_uuid_arg(struct trace_seq *s, const char *ptr, > > Add a space between the variables and the function. > > Also, make them static const: > > static const int [gu]id_index[16] = ... > > > + void *data, int size, struct tep_event *event, > > + struct tep_print_arg *arg) > > +{ > > + int *index = uuid_index; > > + char *format = "%02x"; > > + int ret = 0; > > + char *buf; > > + int i; > > + > > + switch (*(ptr + 1)) { > > + case 'L': > > + format = "%02X"; > > + /* fall through */ > > + case 'l': > > + index = guid_index; > > + ret++; > > + break; > > + case 'B': > > + format = "%02X"; > > + /* fall through */ > > + case 'b': > > + ret++; > > + break; > > + } > > + > > + if (arg->type == TEP_PRINT_FUNC) { > > + process_defined_func(s, data, size, event, arg); > > + return ret; > > + } > > + > > + if (arg->type != TEP_PRINT_FIELD) { > > + trace_seq_printf(s, "ARG TYPE NOT FIELD BUT %d", arg->type); > > + return ret; > > + } > > + > > + if (!arg->field.field) { > > + arg->field.field = > > + tep_find_any_field(event, arg->field.name); > > + if (!arg->field.field) { > > + do_warning("%s: field %s not found", > > + __func__, arg->field.name); > > + return ret; > > + } > > + } > > + > > + if (arg->field.field->size != 16) { > > + trace_seq_printf(s, "INVALIDUUID"); > > + return ret; > > + } > > + > > + buf = data + arg->field.field->offset; > > + > > + for (i = 0; i < 16; i++) { > > + trace_seq_printf(s, format, buf[index[i]] & 0xff); > > + switch (i) { > > + case 3: > > + case 5: > > + case 7: > > + case 9: > > + trace_seq_printf(s, "-"); > > + break; > > + } > > + } > > + > > + return ret; > > +} > > + > > +static int print_raw_buff_arg(struct trace_seq *s, const char *ptr, > > + void *data, int size, struct tep_event *event, > > + struct tep_print_arg *arg, int print_len) > > +{ > > + int plen = print_len; > > + char *delim = " "; > > + int ret = 0; > > + char *buf; > > + int i; > > + unsigned long offset; > > + int arr_len; > > + > > + switch (*(ptr + 1)) { > > + case 'C': > > + delim = ":"; > > + ret++; > > + break; > > + case 'D': > > + delim = "-"; > > + ret++; > > + break; > > + case 'N': > > + delim = ""; > > + ret++; > > + break; > > + } > > + > > + if (arg->type == TEP_PRINT_FUNC) { > > + process_defined_func(s, data, size, event, arg); > > + return ret; > > + } > > + > > + if (arg->type != TEP_PRINT_DYNAMIC_ARRAY) { > > + trace_seq_printf(s, "ARG TYPE NOT FIELD BUT %d", arg->type); > > + return ret; > > Hmm, shouldn't this also be able to work with a constant array? I should work, but I couldn't find any events using this print format with other than dynamic array, in order to test it. We can add other argument types when I implement a test libtraceevent kernel module, so we can test any combinations between print format strings and argument types. > > > + } > > + > > + offset = tep_read_number(event->tep, > > + data + arg->dynarray.field->offset, > > + arg->dynarray.field->size); > > + arr_len = (unsigned long long)(offset >> 16); > > + buf = data + (offset & 0xffff); > > + > > + if (arr_len < plen) > > + plen = arr_len; > > + > > + if (plen < 1) > > + return ret; > > + > > + trace_seq_printf(s, "%02x", buf[0] & 0xff); > > + for (i = 1; i < plen; i++) > > + trace_seq_printf(s, "%s%02x", delim, buf[i] & 0xff); > > + > > + return ret; > > +} > > + > > static int is_printable_array(char *p, unsigned int len) > > { > > unsigned int i; > > @@ -4952,6 +5136,71 @@ void tep_print_fields(struct trace_seq *s, void *data, > > } > > } > > > > +static int print_function(struct trace_seq *s, const char *format, > > + void *data, int size, struct tep_event *event, > > + struct tep_print_arg *arg) > > +{ > > + struct func_map *func; > > + unsigned long long val; > > + > > + val = eval_num_arg(data, size, event, arg); > > + func = find_func(event->tep, val); > > + if (func) { > > + trace_seq_puts(s, func->func); > > + if (*format == 'F' || *format == 'S') > > + trace_seq_printf(s, "+0x%llx", val - func->addr); > > + } else { > > + if (event->tep->long_size == 4) > > + trace_seq_printf(s, "0x%lx", (long)val); > > + else > > + trace_seq_printf(s, "0x%llx", (long long)val); > > + } > > + > > + return 0; > > +} > > + > > +static int print_pointer(struct trace_seq *s, const char *format, int plen, > > + void *data, int size, > > + struct tep_event *event, struct tep_print_arg *arg) > > +{ > > + unsigned long long val; > > + int ret = 1; > > + > > + switch (*format) { > > + case 'F': > > + case 'f': > > + case 'S': > > + case 's': > > + ret += print_function(s, format, data, size, event, arg); > > + break; > > + case 'M': > > + case 'm': > > + ret += print_mac_arg(s, format, data, size, event, arg); > > + break; > > + case 'I': > > + case 'i': > > + ret += print_ip_arg(s, format, data, size, event, arg); > > + break; > > + case 'U': > > + ret += print_uuid_arg(s, format, data, size, event, arg); > > + break; > > + case 'h': > > + ret += print_raw_buff_arg(s, format, data, size, event, arg, plen); > > + break; > > + default: > > + ret = 0; > > + val = eval_num_arg(data, size, event, arg); > > + if (event->tep->long_size == 4) > > + trace_seq_printf(s, "0x%lx", (long)val); > > + else > > + trace_seq_printf(s, "0x%llx", (long long)val); > > + break; > > + } > > + > > + return ret; > > + > > +} > > + > > static void pretty_print(struct trace_seq *s, void *data, int size, struct tep_event *event) > > { > > struct tep_handle *tep = event->tep; > > @@ -4960,16 +5209,15 @@ static void pretty_print(struct trace_seq *s, void *data, int size, struct tep_e > > struct tep_print_arg *args = NULL; > > const char *ptr = print_fmt->format; > > unsigned long long val; > > - struct func_map *func; > > const char *saveptr; > > struct trace_seq p; > > char *bprint_fmt = NULL; > > char format[32]; > > - int show_func; > > int len_as_arg; > > int len_arg = 0; > > int len; > > int ls; > > + int ret; > > > > if (event->flags & TEP_EVENT_FL_FAILED) { > > trace_seq_printf(s, "[FAILED TO PARSE]"); > > @@ -5008,7 +5256,6 @@ static void pretty_print(struct trace_seq *s, void *data, int size, struct tep_e > > > > } else if (*ptr == '%') { > > saveptr = ptr; > > - show_func = 0; > > len_as_arg = 0; > > cont_process: > > ptr++; > > @@ -5046,39 +5293,19 @@ static void pretty_print(struct trace_seq *s, void *data, int size, struct tep_e > > case '-': > > goto cont_process; > > case 'p': > > - if (tep->long_size == 4) > > - ls = 1; > > - else > > - ls = 2; > > - > > - if (isalnum(ptr[1])) > > - ptr++; > > - > > if (arg->type == TEP_PRINT_BSTRING) { > > This will still need the above check. As this is for trace_printk() in > the kernel. And that will record the pointer conversion (except for > functions) into the buffer as a string (hence why it only prints the > string). But that said, the format still needs to be passed, otherwise > it is printed. We should (not in this patch), probably do the full > logic of parsing the format as well, as the check for isalnum() is not > enough. > > -- Steve Thanks, Steven! I'll submit the next patch version addressing these comments. > > > > trace_seq_puts(s, arg->string.string); > > arg = arg->next; > > break; > > } > > - > > - if (*ptr == 'F' || *ptr == 'f' || > > - *ptr == 'S' || *ptr == 's') { > > - show_func = *ptr; > > - } else if (*ptr == 'M' || *ptr == 'm') { > > - print_mac_arg(s, *ptr, data, size, event, arg); > > - arg = arg->next; > > - break; > > - } else if (*ptr == 'I' || *ptr == 'i') { > > - int n; > > - > > - n = print_ip_arg(s, ptr, data, size, event, arg); > > - if (n > 0) { > > - ptr += n - 1; > > - arg = arg->next; > > - break; > > - } > > - } > > - > > - /* fall through */ > > + ret = print_pointer(s, ptr + 1, > > + len_as_arg ? len_arg : 1, > > + data, size, > > + event, arg); > > + arg = arg->next; > > + if (ret > 0) > > + ptr += ret; > > + break; > > case 'd': > > case 'u': > > case 'i': > > @@ -5107,17 +5334,6 @@ static void pretty_print(struct trace_seq *s, void *data, int size, struct tep_e > > val = eval_num_arg(data, size, event, arg); > > arg = arg->next; > > > > - if (show_func) { > > - func = find_func(tep, val); > > - if (func) { > > - trace_seq_puts(s, func->func); > > - if (show_func == 'F') > > - trace_seq_printf(s, > > - "+0x%llx", > > - val - func->addr); > > - break; > > - } > > - } > > if (tep->long_size == 8 && ls == 1 && > > sizeof(long) != 8) { > > char *p; > > @@ -5125,8 +5341,6 @@ static void pretty_print(struct trace_seq *s, void *data, int size, struct tep_e > > /* make %l into %ll */ > > if (ls == 1 && (p = strchr(format, 'l'))) > > memmove(p+1, p, strlen(p)+1); > > - else if (strcmp(format, "%p") == 0) > > - strcpy(format, "0x%llx"); > > ls = 2; > > } > > switch (ls) { > -- Tzvetomir (Ceco) Stoyanov VMware Open Source Technology Center