| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <20070618194301.GA6038@martell.zuzino.mipt.ru>
Date: Mon, 18 Jun 2007 23:43:01 +0400
From: Alexey Dobriyan <adobriyan@...il.com>
To: David Wilder <dwilder@...ibm.com>
Cc: linux-kernel@...r.kernel.org, systemtap@...rces.redhat.com
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@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists