| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <1333750544.2920.6.camel@acer.local.home>
Date: Fri, 06 Apr 2012 18:15:44 -0400
From: Steven Rostedt <rostedt@...dmis.org>
To: Borislav Petkov <bp@...en8.de>
Cc: Frederic Weisbecker <fweisbec@...il.com>,
LKML <linux-kernel@...r.kernel.org>,
Steven Rostedt <srostedt@...hat.com>,
Ingo Molnar <mingo@...nel.org>,
Thomas Gleixner <tglx@...utronix.de>,
Peter Zijlstra <peterz@...radead.org>,
Arnaldo Carvalho de Melo <acme@...radead.org>,
Jiri Olsa <jolsa@...hat.com>, Arun Sharma <asharma@...com>,
Namhyung Kim <namhyung.kim@....com>
Subject: Re: [PATCH 02/15] tools/events: Add files to create libtraceevent.a
On Fri, 2012-04-06 at 15:21 +0200, Borislav Petkov wrote:
> <snip>
>
> Btw, running this through checkpatch generates a load of warnings/errors
> and some of them actually could be worth fixing...
Yeah, there were patches I added without running checkpatch (in the
trace-cmd code).
>
> > diff --git a/tools/lib/traceevent/event-parse.c b/tools/lib/traceevent/event-parse.c
> > new file mode 100644
> > index 0000000..47a3227
> > --- /dev/null
> > +++ b/tools/lib/traceevent/event-parse.c
> > @@ -0,0 +1,4971 @@
> > +/*
> > + * Copyright (C) 2009, 2010 Red Hat Inc, Steven Rostedt <srostedt@...hat.com>
> > + *
> > + * ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> > + * This program is free software; you can redistribute it and/or
> > + * modify it under the terms of the GNU Lesser General Public
> > + * License as published by the Free Software Foundation;
> > + * version 2.1 of the License (not later!)
> > + *
> > + * This program is distributed in the hope that it will be useful,
> > + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> > + * GNU Lesser General Public License for more details.
> > + *
> > + * You should have received a copy of the GNU Lesser General Public
> > + * License along with this program; if not, write to the Free Software
> > + * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA.
> > + *
> > + * ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> > + *
> > + * The parts for function graph printing was taken and modified from the
> > + * Linux Kernel that were written by
> > + * - Copyright (C) 2009 Frederic Weisbecker,
> > + * Frederic Weisbecker gave his permission to relicense the code to
> > + * the Lesser General Public License.
> > + */
> > +#define _GNU_SOURCE
> > +#include <stdio.h>
> > +#include <stdlib.h>
> > +#include <string.h>
> > +#include <stdarg.h>
> > +#include <ctype.h>
> > +#include <errno.h>
> > +
> > +#include "event-parse.h"
> > +
> > +static const char *input_buf;
> > +static unsigned long long input_buf_ptr;
> > +static unsigned long long input_buf_siz;
> > +
> > +static int show_warning = 1;
> > +
> > +#define do_warning(fmt, ...) \
> > + do { \
> > + if (show_warning) \
> > + warning(fmt, ##__VA_ARGS__); \
> > + } while (0)
> > +
>
> Yeah, the name "do_warning" is awkward. Can we call it emit_warning or
> _warning or whatever?
As this is just a macro defined as a helper in this file (not global),
I'm not sure it needs to be changed. "emit" has two more characters to
type over "do" ;-)
>
> > +static void init_input_buf(const char *buf, unsigned long long size)
> > +{
> > + input_buf = buf;
> > + input_buf_siz = size;
> > + input_buf_ptr = 0;
> > +}
> > +
> > +const char *pevent_get_input_buf(void)
> > +{
> > + return input_buf;
> > +}
> > +
> > +unsigned long long pevent_get_input_buf_ptr(void)
> > +{
> > + return input_buf_ptr;
> > +}
> > +
> > +struct event_handler {
> > + struct event_handler *next;
> > + int id;
> > + const char *sys_name;
> > + const char *event_name;
> > + pevent_event_handler_func func;
> > + void *context;
> > +};
> > +
> > +struct pevent_func_params {
> > + struct pevent_func_params *next;
> > + enum pevent_func_arg_type type;
> > +};
> > +
> > +struct pevent_function_handler {
> > + struct pevent_function_handler *next;
> > + enum pevent_func_arg_type ret_type;
> > + char *name;
> > + pevent_func_handler func;
> > + struct pevent_func_params *params;
> > + int nr_args;
> > +};
> > +
> > +static unsigned long long
> > +process_defined_func(struct trace_seq *s, void *data, int size,
> > + struct event_format *event, struct print_arg *arg);
> > +
> > +static void free_func_handle(struct pevent_function_handler *func);
> > +
> > +/**
>
> I see you're using kernel-doc style comments. Is this aimed at providing
> documentation for this library? If so, good idea!
That was the plan. It also forced me to document the API.
>
> > + * pevent_buffer_init - init buffer for parsing
> > + * @buf: buffer to parse
> > + * @size: the size of the buffer
> > + *
> > + * For use with pevent_read_token(), this initializes the internal
> > + * buffer that pevent_read_token() will parse.
> > + */
> > +void pevent_buffer_init(const char *buf, unsigned long long size)
> > +{
> > + init_input_buf(buf, size);
> > +}
>
> Can't we unify pevent_buffer_init and init_input_buf()? I mean, it is
> useless to have that kind of indirection.
Yeah I agree. This was more of a historical artifact. It use to be just
init_input_buf() then I needed to export it for other users. Instead of
doing a full rename, I cheesed out and did this.
>
> In general, is "pevent" the prefix for this library's functions which
> are getting exported to users?
Yep. It's the "namespace" used. But if people do not like it, we can
come up with another name. I just want it to be a short prefix.
>
> > +
> > +void breakpoint(void)
> > +{
> > + static int x;
> > + x++;
> > +}
>
> This looks like a debugging leftover since it is not used anywhere...?
Sorta. I kept it in trace-cmd on purpose, as I would set a breakpoint on
this function, and just manually add this function around the code, and
gdb would break on them. I found this easier than constantly needing to
add a breakpoint somewhere.
That said, it does not belong here and should be removed.
>
> > +
> > +struct print_arg *alloc_arg(void)
> > +{
> > + struct print_arg *arg;
> > +
> > + arg = malloc_or_die(sizeof(*arg));
> > + if (!arg)
> > + return NULL;
> > + memset(arg, 0, sizeof(*arg));
> > +
> > + return arg;
> > +}
> > +
> > +struct cmdline {
> > + char *comm;
> > + int pid;
> > +};
> > +
> > +static int cmdline_cmp(const void *a, const void *b)
> > +{
> > + const struct cmdline *ca = a;
> > + const struct cmdline *cb = b;
> > +
> > + if (ca->pid < cb->pid)
> > + return -1;
> > + if (ca->pid > cb->pid)
> > + return 1;
> > +
> > + return 0;
> > +}
> > +
> > +struct cmdline_list {
> > + struct cmdline_list *next;
> > + char *comm;
> > + int pid;
> > +};
> > +
> > +static int cmdline_init(struct pevent *pevent)
> > +{
> > + struct cmdline_list *cmdlist = pevent->cmdlist;
> > + struct cmdline_list *item;
> > + struct cmdline *cmdlines;
> > + int i;
> > +
> > + cmdlines = malloc_or_die(sizeof(*cmdlines) * pevent->cmdline_count);
> > +
> > + i = 0;
> > + while (cmdlist) {
> > + cmdlines[i].pid = cmdlist->pid;
> > + cmdlines[i].comm = cmdlist->comm;
> > + i++;
> > + item = cmdlist;
> > + cmdlist = cmdlist->next;
> > + free(item);
> > + }
> > +
> > + qsort(cmdlines, pevent->cmdline_count, sizeof(*cmdlines), cmdline_cmp);
> > +
> > + pevent->cmdlines = cmdlines;
> > + pevent->cmdlist = NULL;
> > +
> > + return 0;
> > +}
> > +
> > +static char *find_cmdline(struct pevent *pevent, int pid)
> > +{
> > + const struct cmdline *comm;
> > + struct cmdline key;
> > +
> > + if (!pid)
> > + return "<idle>";
> > +
> > + if (!pevent->cmdlines)
> > + cmdline_init(pevent);
> > +
> > + key.pid = pid;
> > +
> > + comm = bsearch(&key, pevent->cmdlines, pevent->cmdline_count,
> > + sizeof(*pevent->cmdlines), cmdline_cmp);
> > +
> > + if (comm)
> > + return comm->comm;
> > + return "<...>";
> > +}
> > +
> > +/**
> > + * pevent_pid_is_registered - return if a pid has a cmdline registered
> > + * @pevent: handle for the pevent
> > + * @pid: The pid to check if it has a cmdline registered with.
> > + *
> > + * Returns 1 if the pid has a cmdline mapped to it
> > + * 0 otherwise.
> > + */
> > +int pevent_pid_is_registered(struct pevent *pevent, int pid)
> > +{
> > + const struct cmdline *comm;
> > + struct cmdline key;
> > +
> > + if (!pid)
> > + return 1;
> > +
> > + if (!pevent->cmdlines)
> > + cmdline_init(pevent);
> > +
> > + key.pid = pid;
> > +
> > + comm = bsearch(&key, pevent->cmdlines, pevent->cmdline_count,
> > + sizeof(*pevent->cmdlines), cmdline_cmp);
> > +
> > + if (comm)
> > + return 1;
> > + return 0;
> > +}
>
> This one looks almost identical to find_cmdline() above, maybe unify?
Similar but slightly different. Maybe they can be merged, but as they
are so small, it may not matter (for now).
>
> > +
> > +/*
> > + * If the command lines have been converted to an array, then
> > + * we must add this pid. This is much slower than when cmdlines
> > + * are added before the array is initialized.
> > + */
> > +static int add_new_comm(struct pevent *pevent, const char *comm, int pid)
> > +{
> > + struct cmdline *cmdlines = pevent->cmdlines;
> > + const struct cmdline *cmdline;
> > + struct cmdline key;
> > +
> > + if (!pid)
> > + return 0;
> > +
> > + /* avoid duplicates */
> > + key.pid = pid;
> > +
> > + cmdline = bsearch(&key, pevent->cmdlines, pevent->cmdline_count,
> > + sizeof(*pevent->cmdlines), cmdline_cmp);
> > + if (cmdline) {
> > + errno = EEXIST;
> > + return -1;
> > + }
> > +
> > + cmdlines = realloc(cmdlines, sizeof(*cmdlines) * (pevent->cmdline_count + 1));
> > + if (!cmdlines) {
> > + errno = ENOMEM;
> > + return -1;
> > + }
> > +
> > + cmdlines[pevent->cmdline_count].pid = pid;
> > + cmdlines[pevent->cmdline_count].comm = strdup(comm);
> > + if (!cmdlines[pevent->cmdline_count].comm)
> > + die("malloc comm");
> > +
>
> extraneous whitespace
>
> > + if (cmdlines[pevent->cmdline_count].comm)
> > + pevent->cmdline_count++;
> > +
> > + qsort(cmdlines, pevent->cmdline_count, sizeof(*cmdlines), cmdline_cmp);
> > + pevent->cmdlines = cmdlines;
> > +
> > + return 0;
> > +}
> > +
> > +/**
> > + * pevent_register_comm - register a pid / comm mapping
> > + * @pevent: handle for the pevent
> > + * @comm: the command line to register
> > + * @pid: the pid to map the command line to
> > + *
> > + * This adds a mapping to search for command line names with
> > + * a given pid. The comm is duplicated.
> > + */
> > +int pevent_register_comm(struct pevent *pevent, const char *comm, int pid)
> > +{
> > + struct cmdline_list *item;
> > +
> > + if (pevent->cmdlines)
> > + return add_new_comm(pevent, comm, pid);
> > +
> > + item = malloc_or_die(sizeof(*item));
> > + item->comm = strdup(comm);
> > + if (!item->comm)
> > + die("malloc comm");
> > + item->pid = pid;
> > + item->next = pevent->cmdlist;
> > +
> > + pevent->cmdlist = item;
> > + pevent->cmdline_count++;
> > +
> > + return 0;
> > +}
> > +
> > +struct func_map {
> > + unsigned long long addr;
> > + char *func;
> > + char *mod;
> > +};
> > +
> > +struct func_list {
> > + struct func_list *next;
> > + unsigned long long addr;
> > + char *func;
> > + char *mod;
> > +};
> > +
> > +static int func_cmp(const void *a, const void *b)
> > +{
> > + const struct func_map *fa = a;
> > + const struct func_map *fb = b;
> > +
> > + if (fa->addr < fb->addr)
> > + return -1;
> > + if (fa->addr > fb->addr)
> > + return 1;
> > +
> > + return 0;
> > +}
> > +
> > +/*
> > + * We are searching for a record in between, not an exact
> > + * match.
> > + */
> > +static int func_bcmp(const void *a, const void *b)
> > +{
> > + const struct func_map *fa = a;
> > + const struct func_map *fb = b;
> > +
> > + if ((fa->addr == fb->addr) ||
> > +
>
> superfluous newline?
>
> > + (fa->addr > fb->addr &&
> > + fa->addr < (fb+1)->addr))
> > + return 0;
> > +
> > + if (fa->addr < fb->addr)
> > + return -1;
> > +
> > + return 1;
> > +}
> > +
> > +static int func_map_init(struct pevent *pevent)
> > +{
> > + struct func_list *funclist;
> > + struct func_list *item;
> > + struct func_map *func_map;
> > + int i;
> > +
> > + func_map = malloc_or_die(sizeof(*func_map) * (pevent->func_count + 1));
> > + funclist = pevent->funclist;
> > +
> > + i = 0;
> > + while (funclist) {
> > + func_map[i].func = funclist->func;
> > + func_map[i].addr = funclist->addr;
> > + func_map[i].mod = funclist->mod;
> > + i++;
> > + item = funclist;
> > + funclist = funclist->next;
> > + free(item);
> > + }
> > +
> > + qsort(func_map, pevent->func_count, sizeof(*func_map), func_cmp);
> > +
> > + /*
> > + * Add a special record at the end.
> > + */
> > + func_map[pevent->func_count].func = NULL;
> > + func_map[pevent->func_count].addr = 0;
> > + func_map[pevent->func_count].mod = NULL;
> > +
> > + pevent->func_map = func_map;
> > + pevent->funclist = NULL;
> > +
> > + return 0;
> > +}
> > +
> > +static struct func_map *
> > +find_func(struct pevent *pevent, unsigned long long addr)
> > +{
> > + struct func_map *func;
> > + struct func_map key;
> > +
> > + if (!pevent->func_map)
> > + func_map_init(pevent);
> > +
> > + key.addr = addr;
> > +
> > + func = bsearch(&key, pevent->func_map, pevent->func_count,
> > + sizeof(*pevent->func_map), func_bcmp);
> > +
> > + return func;
>
> Simply do:
>
> return bsearch(&key, ...);
That may have been caused by removing code (or adding debug between the
two).
But yeah, that should be simplified.
>
> > +}
> > +
> > +/**
> > + * pevent_find_function - find a function by a given address
> > + * @pevent: handle for the pevent
> > + * @addr: the address to find the function with
> > + *
> > + * Returns a pointer to the function stored that has the given
> > + * address. Note, the address does not have to be exact, it
> > + * will select the function that would contain the address.
> > + */
> > +const char *pevent_find_function(struct pevent *pevent, unsigned long long addr)
> > +{
> > + struct func_map *map;
> > +
> > + map = find_func(pevent, addr);
> > + if (!map)
> > + return NULL;
> > +
> > + return map->func;
> > +}
> > +
> > +/**
> > + * pevent_find_function_address - find a function address by a given address
> > + * @pevent: handle for the pevent
> > + * @addr: the address to find the function with
> > + *
> > + * Returns the address the function starts at. This can be used in
> > + * conjunction with pevent_find_function to print both the function
> > + * name and the function offset.
> > + */
> > +unsigned long long
> > +pevent_find_function_address(struct pevent *pevent, unsigned long long addr)
> > +{
> > + struct func_map *map;
> > +
> > + map = find_func(pevent, addr);
> > + if (!map)
> > + return 0;
> > +
> > + return map->addr;
> > +}
> > +
> > +/**
> > + * pevent_register_function - register a function with a given address
> > + * @pevent: handle for the pevent
> > + * @function: the function name to register
> > + * @addr: the address the function starts at
> > + * @mod: the kernel module the function may be in (NULL for none)
> > + *
> > + * This registers a function name with an address and module.
> > + * The @func passed in is duplicated.
> > + */
> > +int pevent_register_function(struct pevent *pevent, char *func,
> > + unsigned long long addr, char *mod)
> > +{
> > + struct func_list *item;
> > +
> > + item = malloc_or_die(sizeof(*item));
> > +
> > + item->next = pevent->funclist;
> > + item->func = strdup(func);
> > + if (mod)
> > + item->mod = strdup(mod);
> > + else
> > + item->mod = NULL;
> > + item->addr = addr;
> > +
> > + pevent->funclist = item;
> > +
> > + pevent->func_count++;
> > +
> > + return 0;
> > +}
> > +
> > +/**
> > + * pevent_print_funcs - print out the stored functions
> > + * @pevent: handle for the pevent
> > + *
> > + * This prints out the stored functions.
> > + */
> > +void pevent_print_funcs(struct pevent *pevent)
> > +{
> > + int i;
> > +
> > + if (!pevent->func_map)
> > + func_map_init(pevent);
> > +
> > + for (i = 0; i < (int)pevent->func_count; i++) {
>
> simply declare i as "unsigned int" and you don't need the cast here.
I think I did that once but it caused problems elsewhere. But we can
look at it again.
>
> > + printf("%016llx %s",
> > + pevent->func_map[i].addr,
> > + pevent->func_map[i].func);
> > + if (pevent->func_map[i].mod)
> > + printf(" [%s]\n", pevent->func_map[i].mod);
> > + else
> > + printf("\n");
> > + }
> > +}
> > +
> > +struct printk_map {
> > + unsigned long long addr;
> > + char *printk;
> > +};
> > +
> > +struct printk_list {
> > + struct printk_list *next;
> > + unsigned long long addr;
> > + char *printk;
> > +};
> > +
> > +static int printk_cmp(const void *a, const void *b)
> > +{
> > + const struct func_map *fa = a;
> > + const struct func_map *fb = b;
> > +
> > + if (fa->addr < fb->addr)
> > + return -1;
> > + if (fa->addr > fb->addr)
> > + return 1;
> > +
> > + return 0;
> > +}
> > +
> > +static void printk_map_init(struct pevent *pevent)
> > +{
> > + struct printk_list *printklist;
> > + struct printk_list *item;
> > + struct printk_map *printk_map;
> > + int i;
> > +
> > + printk_map = malloc_or_die(sizeof(*printk_map) * (pevent->printk_count + 1));
> > +
> > + printklist = pevent->printklist;
> > +
> > + i = 0;
> > + while (printklist) {
> > + printk_map[i].printk = printklist->printk;
> > + printk_map[i].addr = printklist->addr;
> > + i++;
> > + item = printklist;
> > + printklist = printklist->next;
> > + free(item);
> > + }
> > +
> > + qsort(printk_map, pevent->printk_count, sizeof(*printk_map), printk_cmp);
> > +
> > + pevent->printk_map = printk_map;
> > + pevent->printklist = NULL;
> > +}
> > +
> > +static struct printk_map *
> > +find_printk(struct pevent *pevent, unsigned long long addr)
> > +{
> > + struct printk_map *printk;
> > + struct printk_map key;
> > +
> > + if (!pevent->printk_map)
> > + printk_map_init(pevent);
>
> You could pull those existence tests in the _init function above, i.e:
>
> static void printk_map_init(struct pevent *pevent)
> {
> if (pevent->printk_map)
> return;
>
> so that you can keep the init details to the init functions. Ditto for
> the remaining _init(..) calls.
Sure.
>
> > +
> > + key.addr = addr;
> > +
> > + printk = bsearch(&key, pevent->printk_map, pevent->printk_count,
> > + sizeof(*pevent->printk_map), printk_cmp);
> > +
> > + return printk;
> > +}
> > +
> > +/**
> > + * pevent_register_print_string - register a string by its address
> > + * @pevent: handle for the pevent
> > + * @fmt: the string format to register
> > + * @addr: the address the string was located at
> > + *
> > + * This registers a string by the address it was stored in the kernel.
> > + * The @fmt passed in is duplicated.
> > + */
> > +int pevent_register_print_string(struct pevent *pevent, char *fmt,
> > + unsigned long long addr)
> > +{
> > + struct printk_list *item;
> > +
> > + item = malloc_or_die(sizeof(*item));
> > +
> > + item->next = pevent->printklist;
> > + pevent->printklist = item;
> > + item->printk = strdup(fmt);
> > + item->addr = addr;
> > +
> > + pevent->printk_count++;
> > +
> > + return 0;
> > +}
> > +
> > +/**
> > + * pevent_print_printk - print out the stored strings
> > + * @pevent: handle for the pevent
> > + *
> > + * This prints the string formats that were stored.
> > + */
> > +void pevent_print_printk(struct pevent *pevent)
> > +{
> > + int i;
> > +
> > + if (!pevent->printk_map)
> > + printk_map_init(pevent);
>
> ditto.
>
> > +
> > + for (i = 0; i < (int)pevent->printk_count; i++) {
>
> ditto for making i an unsigned int.
>
> > + printf("%016llx %s\n",
> > + pevent->printk_map[i].addr,
> > + pevent->printk_map[i].printk);
> > + }
> > +}
> > +
> > +static struct event_format *alloc_event(void)
> > +{
> > + struct event_format *event;
> > +
> > + event = malloc_or_die(sizeof(*event));
> > + memset(event, 0, sizeof(*event));
>
> perf (or tools/) probably needs a zalloc_or_die which memset's the ptr
> to 0 - this'll save a bunch of code.
Yeah, agreed. I've just been too lazy to add them ;-)
>
> > +
> > + return event;
> > +}
> > +
> > +static void add_event(struct pevent *pevent, struct event_format *event)
> > +{
> > + int i;
> > +
> > + if (!pevent->events)
> > + pevent->events = malloc_or_die(sizeof(event));
> > + else
> > + pevent->events =
> > + realloc(pevent->events, sizeof(event) *
> > + (pevent->nr_events + 1));
> > + if (!pevent->events)
> > + die("Can not allocate events");
> > +
> > + for (i = 0; i < pevent->nr_events; i++) {
> > + if (pevent->events[i]->id > event->id)
> > + break;
> > + }
> > + if (i < pevent->nr_events)
> > + memmove(&pevent->events[i + 1],
> > + &pevent->events[i],
> > + sizeof(event) * (pevent->nr_events - i));
> > +
> > + pevent->events[i] = event;
> > + pevent->nr_events++;
> > +
> > + event->pevent = pevent;
> > +}
> > +
> > +static int event_item_type(enum event_type type)
> > +{
> > + switch (type) {
> > + case EVENT_ITEM ... EVENT_SQUOTE:
> > + return 1;
> > + case EVENT_ERROR ... EVENT_DELIM:
> > + default:
> > + return 0;
> > + }
> > +}
> > +
> > +static void free_flag_sym(struct print_flag_sym *fsym)
> > +{
> > + struct print_flag_sym *next;
> > +
> > + while (fsym) {
> > + next = fsym->next;
> > + free(fsym->value);
> > + free(fsym->str);
> > + free(fsym);
> > + fsym = next;
> > + }
> > +}
> > +
> > +static void free_arg(struct print_arg *arg)
> > +{
> > + struct print_arg *farg;
> > +
> > + if (!arg)
> > + return;
> > +
> > + switch (arg->type) {
> > + case PRINT_ATOM:
> > + free(arg->atom.atom);
> > + break;
> > + case PRINT_FIELD:
> > + free(arg->field.name);
> > + break;
> > + case PRINT_FLAGS:
> > + free_arg(arg->flags.field);
> > + free(arg->flags.delim);
> > + free_flag_sym(arg->flags.flags);
> > + break;
> > + case PRINT_SYMBOL:
> > + free_arg(arg->symbol.field);
> > + free_flag_sym(arg->symbol.symbols);
> > + break;
> > + case PRINT_TYPE:
> > + free(arg->typecast.type);
> > + free_arg(arg->typecast.item);
> > + break;
> > + case PRINT_STRING:
> > + case PRINT_BSTRING:
> > + free(arg->string.string);
> > + break;
> > + case PRINT_DYNAMIC_ARRAY:
> > + free(arg->dynarray.index);
> > + break;
> > + case PRINT_OP:
> > + free(arg->op.op);
> > + free_arg(arg->op.left);
> > + free_arg(arg->op.right);
> > + break;
> > + case PRINT_FUNC:
> > + while (arg->func.args) {
> > + farg = arg->func.args;
> > + arg->func.args = farg->next;
> > + free_arg(farg);
> > + }
> > + break;
> > +
> > + case PRINT_NULL:
> > + default:
> > + break;
> > + }
> > +
> > + free(arg);
> > +}
> > +
> > +static enum event_type get_type(int ch)
> > +{
> > + if (ch == '\n')
> > + return EVENT_NEWLINE;
> > + if (isspace(ch))
> > + return EVENT_SPACE;
> > + if (isalnum(ch) || ch == '_')
> > + return EVENT_ITEM;
> > + if (ch == '\'')
> > + return EVENT_SQUOTE;
> > + if (ch == '"')
> > + return EVENT_DQUOTE;
> > + if (!isprint(ch))
> > + return EVENT_NONE;
> > + if (ch == '(' || ch == ')' || ch == ',')
> > + return EVENT_DELIM;
> > +
> > + return EVENT_OP;
> > +}
> > +
> > +static int __read_char(void)
> > +{
> > + if (input_buf_ptr >= input_buf_siz)
> > + return -1;
> > +
> > + return input_buf[input_buf_ptr++];
> > +}
> > +
> > +static int __peek_char(void)
> > +{
> > + if (input_buf_ptr >= input_buf_siz)
> > + return -1;
> > +
> > + return input_buf[input_buf_ptr];
> > +}
> > +
> > +/**
> > + * pevent_peek_char - peek at the next character that will be read
> > + *
> > + * Returns the next character read, or -1 if end of buffer.
> > + */
> > +int pevent_peek_char(void)
> > +{
> > + return __peek_char();
> > +}
> > +
> > +static enum event_type force_token(const char *str, char **tok);
> > +
> > +static enum event_type __read_token(char **tok)
> > +{
> > + char buf[BUFSIZ];
> > + int ch, last_ch, quote_ch, next_ch;
> > + int i = 0;
> > + int tok_size = 0;
> > + enum event_type type;
> > +
> > + *tok = NULL;
> > +
> > +
> > + ch = __read_char();
> > + if (ch < 0)
> > + return EVENT_NONE;
> > +
> > + type = get_type(ch);
> > + if (type == EVENT_NONE)
> > + return type;
> > +
> > + buf[i++] = ch;
> > +
> > + switch (type) {
> > + case EVENT_NEWLINE:
> > + case EVENT_DELIM:
> > + *tok = malloc_or_die(2);
> > + (*tok)[0] = ch;
> > + (*tok)[1] = 0;
> > + return type;
> > +
> > + case EVENT_OP:
> > + switch (ch) {
> > + case '-':
> > + next_ch = __peek_char();
> > + if (next_ch == '>') {
> > + buf[i++] = __read_char();
> > + break;
> > + }
> > + /* fall through */
> > + case '+':
> > + case '|':
> > + case '&':
> > + case '>':
> > + case '<':
> > + last_ch = ch;
> > + ch = __peek_char();
> > + if (ch != last_ch)
> > + goto test_equal;
> > + buf[i++] = __read_char();
> > + switch (last_ch) {
> > + case '>':
> > + case '<':
> > + goto test_equal;
> > + default:
> > + break;
> > + }
> > + break;
> > + case '!':
> > + case '=':
> > + goto test_equal;
> > + default: /* what should we do instead? */
> > + break;
> > + }
> > + buf[i] = 0;
> > + *tok = strdup(buf);
> > + return type;
> > +
> > + test_equal:
> > + ch = __peek_char();
> > + if (ch == '=')
> > + buf[i++] = __read_char();
> > + goto out;
> > +
> > + case EVENT_DQUOTE:
> > + case EVENT_SQUOTE:
> > + /* don't keep quotes */
> > + i--;
> > + quote_ch = ch;
> > + last_ch = 0;
> > + concat:
> > + do {
> > + if (i == (BUFSIZ - 1)) {
> > + buf[i] = 0;
> > + if (*tok) {
> > + *tok = realloc(*tok, tok_size + BUFSIZ);
> > + if (!*tok)
> > + return EVENT_NONE;
> > + strcat(*tok, buf);
> > + } else
> > + *tok = strdup(buf);
> > +
> > + if (!*tok)
> > + return EVENT_NONE;
> > + tok_size += BUFSIZ;
> > + i = 0;
> > + }
> > + last_ch = ch;
> > + ch = __read_char();
> > + buf[i++] = ch;
> > + /* the '\' '\' will cancel itself */
> > + if (ch == '\\' && last_ch == '\\')
> > + last_ch = 0;
> > + } while (ch != quote_ch || last_ch == '\\');
> > + /* remove the last quote */
> > + i--;
> > +
> > + /*
> > + * For strings (double quotes) check the next token.
> > + * If it is another string, concatinate the two.
> > + */
> > + if (type == EVENT_DQUOTE) {
> > + unsigned long long save_input_buf_ptr = input_buf_ptr;
> > +
> > + do {
> > + ch = __read_char();
> > + } while (isspace(ch));
> > + if (ch == '"')
> > + goto concat;
> > + input_buf_ptr = save_input_buf_ptr;
> > + }
> > +
> > + goto out;
> > +
> > + case EVENT_ERROR ... EVENT_SPACE:
> > + case EVENT_ITEM:
> > + default:
> > + break;
> > + }
> > +
> > + while (get_type(__peek_char()) == type) {
> > + if (i == (BUFSIZ - 1)) {
> > + buf[i] = 0;
> > + if (*tok) {
> > + *tok = realloc(*tok, tok_size + BUFSIZ);
> > + if (!*tok)
> > + return EVENT_NONE;
> > + strcat(*tok, buf);
> > + } else
> > + *tok = strdup(buf);
> > +
> > + if (!*tok)
> > + return EVENT_NONE;
> > + tok_size += BUFSIZ;
> > + i = 0;
> > + }
> > + ch = __read_char();
> > + buf[i++] = ch;
> > + }
> > +
> > + out:
> > + buf[i] = 0;
> > + if (*tok) {
> > + *tok = realloc(*tok, tok_size + i);
> > + if (!*tok)
> > + return EVENT_NONE;
> > + strcat(*tok, buf);
> > + } else
> > + *tok = strdup(buf);
> > + if (!*tok)
> > + return EVENT_NONE;
> > +
> > + if (type == EVENT_ITEM) {
> > + /*
> > + * Older versions of the kernel has a bug that
> > + * creates invalid symbols and will break the mac80211
> > + * parsing. This is a work around to that bug.
> > + *
> > + * See Linux kernel commit:
> > + * 811cb50baf63461ce0bdb234927046131fc7fa8b
> > + */
> > + if (strcmp(*tok, "LOCAL_PR_FMT") == 0) {
> > + free(*tok);
> > + *tok = NULL;
> > + return force_token("\"\%s\" ", tok);
> > + } else if (strcmp(*tok, "STA_PR_FMT") == 0) {
> > + free(*tok);
> > + *tok = NULL;
> > + return force_token("\" sta:%pM\" ", tok);
> > + } else if (strcmp(*tok, "VIF_PR_FMT") == 0) {
> > + free(*tok);
> > + *tok = NULL;
> > + return force_token("\" vif:%p(%d)\" ", tok);
> > + }
> > + }
> > +
> > + return type;
> > +}
> > +
> > +static enum event_type force_token(const char *str, char **tok)
> > +{
> > + const char *save_input_buf;
> > + unsigned long long save_input_buf_ptr;
> > + unsigned long long save_input_buf_siz;
> > + enum event_type type;
> > +
>
> Superfluous \t
>
> > + /* save off the current input pointers */
> > + save_input_buf = input_buf;
> > + save_input_buf_ptr = input_buf_ptr;
> > + save_input_buf_siz = input_buf_siz;
> > +
> > + init_input_buf(str, strlen(str));
> > +
> > + type = __read_token(tok);
> > +
> > + /* reset back to original token */
> > + input_buf = save_input_buf;
> > + input_buf_ptr = save_input_buf_ptr;
> > + input_buf_siz = save_input_buf_siz;
> > +
> > + return type;
> > +}
> > +
> > +static void free_token(char *tok)
> > +{
> > + if (tok)
> > + free(tok);
> > +}
>
> According to MALLOC(3), free() can stomach NULL ptrs: "If ptr is NULL,
> no operation is performed."
Hmm, I know that. I wonder why I did it that way?
>
> > +
> > +static enum event_type read_token(char **tok)
> > +{
> > + enum event_type type;
> > +
> > + for (;;) {
> > + type = __read_token(tok);
> > + if (type != EVENT_SPACE)
> > + return type;
> > +
> > + free_token(*tok);
> > + }
> > +
> > + /* not reached */
> > + *tok = NULL;
> > + return EVENT_NONE;
> > +}
> > +
> > +/**
> > + * pevent_read_token - access to utilites to use the pevent parser
> > + * @tok: The token to return
> > + *
> > + * This will parse tokens from the string given by
> > + * pevent_init_data().
> > + *
> > + * Returns the token type.
> > + */
> > +enum event_type pevent_read_token(char **tok)
> > +{
> > + return read_token(tok);
> > +}
>
> Same here: can't we remove all those wrappers and have the library
> routines be called "pevent_" or whatever and all users of the library
> simply call them?
Sure, we can. I probably did this just to keep the typing down ;-)
>
> > +
> > +/**
> > + * pevent_free_token - free a token returned by pevent_read_token
> > + * @token: the token to free
> > + */
> > +void pevent_free_token(char *token)
> > +{
> > + free_token(token);
> > +}
>
> Ditto.
Ditto too!
>
> > +
> > +/* no newline */
>
> Please elaborate. Ah, event type cannot be NEWLINE additionally... Well,
> if that enum event_type were bits, you could unify this function with
> the read_token() above.
Perhaps, but that change is for another day.
>
> > +static enum event_type read_token_item(char **tok)
> > +{
> > + enum event_type type;
> > +
> > + for (;;) {
> > + type = __read_token(tok);
> > + if (type != EVENT_SPACE && type != EVENT_NEWLINE)
> > + return type;
> > + free_token(*tok);
> > + *tok = NULL;
> > + }
> > +
> > + /* not reached */
> > + *tok = NULL;
> > + return EVENT_NONE;
> > +}
>
> Ok, enough for today. I'll review the rest only if you really want me to
> :-).
>
Sure thanks for the comments now. The checkpatch clean ups should
probably be fixed before this goes in, and removing the breakpoint
function. But the other changes probably should wait. I would like what
goes in to be as close to what is in trace-cmd as possible. Perhaps, we
should add the cleanups as a separate patch. Just to have a nice
transition.
Thanks!
-- Steve
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists