This is the mail archive of the
systemtap@sourceware.org
mailing list for the systemtap project.
Re: [RFC] Generic Trace Setup and Control (GTSC) kernel API (2/3)
- From: Alexey Dobriyan <adobriyan at gmail dot com>
- To: David Wilder <dwilder at us dot ibm dot com>
- Cc: linux-kernel at vger dot kernel dot org, systemtap at sources dot redhat dot com
- Date: Mon, 18 Jun 2007 23:43:01 +0400
- Subject: Re: [RFC] Generic Trace Setup and Control (GTSC) kernel API (2/3)
- Dkim-signature: a=rsa-sha1; c=relaxed/relaxed; d=gmail.com; s=beta; h=domainkey-signature:received:received:received:date:from:to:cc:subject:message-id:references:mime-version:content-type:content-disposition:in-reply-to:user-agent; b=uVI16mS62I7LYlgA+ZKeLWD9qejdAL1iE5/139tBR8saFNAO6C/mJhR05HqbaICm8RkYu3JKfm+u7Q7FPWVF6+THW/Z4tZoo9u6WtfamdN7EC6qpwHcxr3g7pKEI1SEr3h6fgAMUJdrISKPuGxvBcb7b/a5adfVT3tKmscAo97E=
- Domainkey-signature: a=rsa-sha1; c=nofws; d=gmail.com; s=beta; h=received:date:from:to:cc:subject:message-id:references:mime-version:content-type:content-disposition:in-reply-to:user-agent; b=tq1KLAb8EnMlE3OEKuBVS3yiy6xKmn8/bw9oj/oVDpcmz3paTG9pBw5quz0yKRV95pAWF5fZDiiNqbDoP7nlrd3hPCZ9bcOhhjW8oAtA+fQyh9HlkcTDqF+7M3mw2F5l1peW1qYmQtuWGwfHjQIhHZ4Qm99qo0Ts6wo6E8HVR6Q=
- References: <4676108B.9030203@us.ibm.com>
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.