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 [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Reply via email to