(2013/07/30 1:40), Tom Zanussi wrote:
> +struct event_command {
> + struct list_head list;
> + char *name;
> + enum trigger_mode trigger_mode;
> + bool post_trigger;
> + int (*func)(struct event_command *cmd_ops,
> + void *cmd_data, char *glob, char *cmd,
> + char *params, int enable);
> + int (*reg)(char *glob,
> + struct event_trigger_ops *trigger_ops,
> + void *trigger_data, void *cmd_data);
> + void (*unreg)(char *glob,
> + struct event_trigger_ops *trigger_ops,
> + void *trigger_data, void *cmd_data);
> + int (*set_filter)(char *filter_str,
> + void *trigger_data,
> + void *cmd_data);
I think you should pass trace_event_file *file (see below) instead of ambiguous
void *cmd_data, because all handler implementations expect that.
[...]
> +#include <linux/module.h>
> +#include <linux/ctype.h>
> +#include <linux/mutex.h>
> +#include <linux/slab.h>
> +
> +#include "trace.h"
> +
> +static LIST_HEAD(trigger_commands);
> +static DEFINE_MUTEX(trigger_cmd_mutex);
> +
> +struct event_trigger_data {
> + struct ftrace_event_file *file;
> + unsigned long count;
> + int ref;
> + bool enable;
> + struct event_trigger_ops *ops;
> + enum trigger_mode mode;
> + struct event_filter *filter;
> + char *filter_str;
> + struct list_head list;
> +};
> +
> +struct trigger_iterator {
> + struct ftrace_event_file *file;
> +};
Why would you define this trigger_iterator even if it has
only one member? This means all iterators can be replaced
by the file. I'd like to keep it simple.
> +
> +void event_triggers_call(struct ftrace_event_file *file)
> +{
> + struct event_trigger_data *data;
> +
> + if (list_empty(&file->triggers))
> + return;
> +
> + preempt_disable_notrace();
> + list_for_each_entry_rcu(data, &file->triggers, list)
> + data->ops->func((void **)&data);
> + preempt_enable_notrace();
> +}
> +EXPORT_SYMBOL_GPL(event_triggers_call);
> +
> +static void *trigger_next(struct seq_file *m, void *t, loff_t *pos)
> +{
> + struct trigger_iterator *iter = m->private;
> +
> + return seq_list_next(t, &iter->file->triggers, pos);
> +}
> +
> +static void *trigger_start(struct seq_file *m, loff_t *pos)
> +{
> + struct trigger_iterator *iter = m->private;
> +
> + mutex_lock(&event_mutex);
> +
> + return seq_list_start(&iter->file->triggers, *pos);
> +}
By Oleg's bugfixes, we are now using event_file_data(filp)
please refer the f_start/trace_format_open implementation
which is the closest usage of the trigger file.
> +static int event_trigger_regex_open(struct inode *inode, struct file *file)
> +{
> + struct trigger_iterator *iter;
> + int ret = 0;
> +
> + iter = kzalloc(sizeof(*iter), GFP_KERNEL);
> + if (!iter)
> + return -ENOMEM;
> +
> + mutex_lock(&event_mutex);
> +
> + iter->file = inode->i_private;
> +
> + if (file->f_mode & FMODE_READ) {
> + ret = seq_open(file, &event_triggers_seq_ops);
> + if (!ret) {
> + struct seq_file *m = file->private_data;
> + m->private = iter;
> + } else {
> + /* Failed */
> + kfree(iter);
> + }
> + } else
> + file->private_data = iter;
> +
> + mutex_unlock(&event_mutex);
> +
> + return ret;
> +}
As you can see in trace_format_open(), now file->private_data and
m->private will point struct file *, don't need to allocate something.
> +static ssize_t event_trigger_regex_write(struct file *file,
> + const char __user *ubuf,
> + size_t cnt, loff_t *ppos, int enable)
> +{
> + struct trigger_iterator *iter = file->private_data;
> + ssize_t ret;
> + char *buf;
> +
> + if (!cnt)
> + return 0;
> +
> + if (cnt >= PAGE_SIZE)
> + return -EINVAL;
> +
> + if (file->f_mode & FMODE_READ) {
> + struct seq_file *m = file->private_data;
> + iter = m->private;
> + } else
> + iter = file->private_data;
> +
> + buf = (char *)__get_free_page(GFP_TEMPORARY);
> + if (!buf)
> + return -ENOMEM;
> +
> + if (copy_from_user(buf, ubuf, cnt)) {
> + free_page((unsigned long) buf);
> + return -EFAULT;
> + }
> + buf[cnt] = '\0';
> + strim(buf);
> +
> + ret = trigger_process_regex(iter, buf, enable);
You also need to get event_mutex while processing event_file to
prevent unexpected releasing.
> +
> + free_page((unsigned long) buf);
> + if (ret < 0)
> + goto out;
> +
> + *ppos += cnt;
> + ret = cnt;
> + out:
> + return ret;
> +}
Thank you,
--
Masami HIRAMATSU
IT Management Research Dept. Linux Technology Center
Hitachi, Ltd., Yokohama Research Laboratory
E-mail: masami.hiramatsu...@hitachi.com
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
