Re: [PATCH, RFC] panic-note: Annotation from user space for panics

[Artem Bityutskiy <dedekind1@gmail.com>]

A pair of nit-picks.

On Wed, 2009-11-11 at 21:13 -0500, David VomLehn wrote:
> @@ -0,0 +1,293 @@
> +/*
> + *				panic-note.c

No need to type file name there.

> + *
> + * Allow a blob to be registered with the kernel that will be printed if
> + * the kernel panics.
> + *
> + * Copyright (C) 2009  Cisco Systems, Inc.
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program; if not, write to the Free Software
> + * Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA  02110-1301  USA
> + */
> +
> +/* Open issues:
> + * Where should the panic_note file be created? It's almost like a sysctl,
> + * but doesn't follow the same rules. When you write to a sysctl file, the
> + * previous data is replaced. When you write to the panic_note file, you
> + * can append to the end of the existing data.
> + */

Please, take a look at what is the kernel comment style at
Documentation/CodingStyle. We use

/*
 * Multi-line
 * comment
 */

style.

> +
> +#include <linux/semaphore.h>
> +#include <linux/fs.h>
> +#include <linux/proc_fs.h>
> +#include <linux/module.h>
> +#include <linux/uaccess.h>
> +
> +/* Maximum size, in bytes, allowed for the blob. Having this limit prevents
> + * an inadvertant denial of service attack that might happen if someone with
> + * root privileges was automatically generating this note and the generator
> + * had an infinite loop. Perhaps this is more of a a denial of service
> + * suicide. */
> +#define PANIC_NOTE_SIZE		(PAGE_SIZE * 4)
> +
> +/*
> + * struct panic_note_data - Information about the panic note
> + * @n:		Number of bytes in the note
> + * @p:		Pointer to the data in the note
> + * @sem:	Semaphore controlling access to data in the note
> + */
> +struct panic_note_state {
> +	size_t			n;
> +	void			*p;
> +	struct rw_semaphore	sem;
> +};
> +
> +static struct panic_note_state panic_note_state = {
> +	0, NULL, __RWSEM_INITIALIZER(panic_note_state.sem)
> +};
> +static const struct file_operations panic_note_fops;
> +static struct inode_operations panic_note_iops;
> +static struct proc_dir_entry *panic_note_entry;
> +
> +/*
> + * panic_note_print - display the panic note
> + * @priority:	Printk priority to use, e.g. KERN_EMERG
> + */
> +void panic_note_print()
> +{
> +	int i;
> +	int linelen;
int i, lineline is more compact.

> +
> +	/* We skip the semaphore stuff because we're in a panic situation and
> +	 * the scheduler isn't available in case the semaphore is already owned
> +	 * by someone else */
> +	for (i = 0; i < panic_note_state.n; i += linelen) {
> +		const char *p;
> +		int remaining;
> +		const char *nl;

const char *p, *nl is more compact. And there are several more places.
But this is matter of taste, really.

> +
> +		p = panic_note_state.p + i;
> +		remaining = panic_note_state.n - i;
> +
> +		nl = memchr(p, '\n', remaining);
> +
> +		if (nl == NULL) {
> +			linelen = remaining;
> +			pr_emerg("%.*s\n", linelen, p);
> +		} else {
> +			linelen = nl - p + 1;
> +			pr_emerg("%.*s", linelen, p);
> +		}
> +	}
> +}
> +
> +/*
> + * read_write_size - calculate the limited copy_to_user/copy_from_user count
> + * @nbytes:	The number of bytes requested
> + * @pos:	Offset, in bytes, into the file
> + * @size:	Maximum I/O offset, in bytes. For a read, this is the actual
> + *		number of bytes in the file, since you can't read past
> + *		the end. Writes can be done after the number of bytes in the
> + *		file, so this is the maximum possible file size, minus one.
> + *
> + * Returns the number of bytes to copy.
> + */
> +static ssize_t read_write_size(size_t nbytes, loff_t pos, size_t size)
> +{
> +	ssize_t retval;
> +
> +	if (pos >= size)
> +		retval = 0;
> +
Unnecessary \n
> +	else {
> +		retval = size - pos;
> +		if (retval > nbytes)
> +			retval = nbytes;
> +	}
> +
> +	return retval;
> +}


-- 
Best Regards,
Artem Bityutskiy (Артём Битюцкий)