Messages in this thread | | | Date | Mon, 18 Jun 2007 23:43:01 +0400 | From | Alexey Dobriyan <> | Subject | Re: [RFC] Generic Trace Setup and Control (GTSC) kernel API (2/3) |
| |
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.
- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
| |