--
David Wilder
IBM Linux Technology Center
Beaverton, Oregon, USA
[email protected]
(503)578-3789
On Sun, Jun 17, 2007 at 09:56:43PM -0700, David Wilder wrote:
> This patch introduces the Generic Trace Setup and Control (GTSC) API.
> In the kernel, GTSC provides a simple API for starting and managing
> data channels to user space. GTSC builds on the relay interface.
My main grief is names choosing. gtsc_ prefixes are everywhere.
In fact doing s/gtsc//g on this patchset would produce better patch.
See below.
> --- /dev/null
> +++ b/include/linux/gtsc.h
> +/*
> + * GTSC channel flags
> + */
> +#define GTSC_GLOBAL 0x01
> +#define GTSC_FLIGHT 0x02
The fact that is channel flags is not deducable.
> +enum {
> + Gtsc_trace_setup = 1,
Starting from zero seems unimportant. You check for equality anyway.
Or not?
> + Gtsc_trace_running,
> + Gtsc_trace_stopped,
> +};
All uppercase would be better.
> +/*
> + * Global root user information
> + */
> +struct gtsc_root {
> + struct list_head list;
> + char gtsc_name[GTSC_TRACE_ROOT_NAME_SIZE];
> + struct dentry *gtsc_root;
> + unsigned int gtsc_users;
> +};
> +
> +/*
> + * Client information
> + */
> +struct gtsc_trace {
> + int trace_state;
named enum, please.
> + struct dentry *state_file;
> + struct rchan *rchan;
> + struct dentry *dir;
> + struct dentry *dropped_file;
> + atomic_t dropped;
> + struct gtsc_root *root;
> + void *private_data;
> + unsigned int flags;
> + unsigned int buf_size;
> + unsigned int buf_nr;
> +};
> +
> +static inline int gtsc_trace_running(struct gtsc_trace *gtsc)
> +{
> + return gtsc->trace_state == Gtsc_trace_running;
> +}
Like here. Compare to
static inline int trace_running(struct trace *trace)
{
return trace->state == TRACE_RUNNING;
}
It's about traces not naming your particular project.
> +#if defined(CONFIG_GTSC)
#ifdef CONFIG_* usually
> +/**
> + * gtsc_trace_startstop: start or stop tracing.
> + *
> + * @gtsc: gtsc trace handle to start or stop.
> + * @start: set to 1 to start tracing set to 0 to stop.
> + *
> + * returns 0 if successful.
> + */
> +extern int gtsc_trace_startstop(struct gtsc_trace *gtsc, int start);
Two functions in one.
> +/**
> + * gtsc_timestamp: returns a time stamp.
> + */
> +extern unsigned long long gtsc_timestamp(void);
Isn't it obvious from name that timestamp is returned?
> +#else /* !CONFIG_GTSC */
> +#define gtsc_trace_setup(root, name, buf_size, buf_nr, flags) (NULL)
> +#define gtsc_trace_startstop(gtsc, start) (-EINVAL)
> +#define gtsc_trace_cleanup(gtsc) do { } while (0)
> +#define gtsc_timestamp(void) (unsigned long long) (0)
static inlines, so those "foo is unused" warnings would not appear.
> +config GTSC
> + bool "Generic Trace Setup and Control"
> + select RELAY
> + select DEBUG_FS
> + help
> + This option enables support for the GTSC.
too small help text and trailing whitespace.
Help texts are usually indented like
Tabhelp
TabSpaceSpace[actual text]
TabSpaceSpace[more text]
> --- /dev/null
> +++ b/lib/gtsc.c
> +static ssize_t gtsc_state_write(struct file *filp, const char __user *buffer,
> + size_t count, loff_t *ppos)
> +{
> + struct gtsc_trace *gtsc = filp->private_data;
> + char buf[16];
> + int ret;
> +
> + memset(buf, 0, sizeof(buf));
> +
> + if (count > sizeof(buf))
> + return -EINVAL;
> +
> + if (copy_from_user(buf, buffer, count))
> + return -EFAULT;
This is too dangerous to leave without explicit placing of terminator.
Think someone will copy it without strncmp() usage.
> + if (strncmp(buf, "start", strlen("start")) == 0 ) {
> + ret = gtsc_trace_startstop(gtsc, 1);
> + if (ret)
> + return ret;
> + } else if (strncmp(buffer, "stop", strlen("stop")) == 0) {
> + ret = gtsc_trace_startstop(gtsc, 0);
> + if (ret)
> + return ret;
> + } else
> + return -EINVAL;
> +
> + return count;
> +}
> +
> +
> +static struct file_operations gtsc_state_fops = {
> + .owner = THIS_MODULE,
> + .open = gtsc_state_open,
> + .read = gtsc_state_read,
> + .write = gtsc_state_write,
[Tab]=[Space][method], is nicer ;-)
> +static struct file_operations gtsc_dropped_fops = {
> + .owner = THIS_MODULE,
> + .open = gtsc_dropped_open,
> + .read = gtsc_dropped_read,
> +};
> +int gtsc_trace_startstop(struct gtsc_trace *gtsc, int start)
> +{
> + int ret = -EINVAL;
> + /*
> + * For starting a trace, we can transition from a setup or stopped
> + * trace. For stopping a trace, the state must be running
> + */
> + if (start) {
> + if (gtsc->trace_state == Gtsc_trace_setup) {
> + ret = gtsc_trace_setup_channel(gtsc, gtsc->buf_size,
> + gtsc->buf_nr,
> + gtsc->flags);
> + if (ret)
> + return ret;
> + }
> + gtsc->trace_state = Gtsc_trace_running;
> + } else {
> + if (gtsc->trace_state == Gtsc_trace_running) {
> + gtsc->trace_state = Gtsc_trace_stopped;
> + relay_flush(gtsc->rchan);
> + }
> + }
> +
> + return 0;
> +}
> +EXPORT_SYMBOL_GPL(gtsc_trace_startstop);
Can we get another user to justify this generalizing?
I also ask to remove extern from prototypes, making .h something like
20 lines long and move kernel-doc comments to *.c file.
On Mon, 2007-06-18 at 23:43 +0400, Alexey Dobriyan wrote:
>
> Can we get another user to justify this generalizing?
Systemtap will be using this. Most users of the relay interface will
need to duplicate much of the functionality of GTSC so I expect others
will use it when it is available.
I forgot to CC the list in my response to Alexey.
I plan to address Alexey's concerns in a couple of days (as soon as I
get past the OLS push).
Alexey Dobriyan wrote:
>Can we get another user to justify this generalizing?
>
>
>
>
Systemtap has plans to use the GTSC also.
--
David Wilder
IBM Linux Technology Center
Beaverton, Oregon, USA
[email protected]
(503)578-3789