Re: [PATCH 1/1] sysfs: add more info to the oops dump

[Greg KH <gregkh@suse.de>]

On Thu, Mar 10, 2011 at 05:53:08PM +0200, Phil Carmody wrote:
> If we're going to remember which file we accessed, we might as well
> also remember whether it was a read or a write, and if the latter,
> some indication of what was written.
> 
> e.g.
> $ echo 1 > /sys/kernel/slab/:at-0000064/sanity_checks
> $ echo c > /proc/sysrq-trigger
> ...
> [  112.457580] last sysfs file (w): /sys/kernel/slab/:at-0000064/sanity_checks
> [  112.464569]   written: 1

Has this actually helped you out?  I've been thinking of removing this
line in the dmesg entirely as I haven't seen it help in a very long time
to track anything down.

> 
> Signed-off-by: Phil Carmody <ext-phil.2.carmody@nokia.com>
> ---
>  fs/sysfs/file.c |   21 ++++++++++++++++++++-
>  1 files changed, 20 insertions(+), 1 deletions(-)
> 
> diff --git a/fs/sysfs/file.c b/fs/sysfs/file.c
> index da3fefe..281e4dd 100644
> --- a/fs/sysfs/file.c
> +++ b/fs/sysfs/file.c
> @@ -26,9 +26,18 @@
>  
>  /* used in crash dumps to help with debugging */
>  static char last_sysfs_file[PATH_MAX];
> +static char last_sysfs_write[16];
> +static int last_sysfs_write_len; /* magic value < 0 => read */
>  void sysfs_printk_last_file(void)
>  {
> -	printk(KERN_EMERG "last sysfs file: %s\n", last_sysfs_file);
> +	printk(KERN_EMERG "last sysfs file (%c): %s\n",
> +	       (last_sysfs_write_len < 0) ? 'r' : 'w',
> +	       last_sysfs_file);
> +	if (last_sysfs_write_len >= sizeof(last_sysfs_write))
> +		printk(KERN_EMERG "  written: %s...(%d chars)\n",
> +		       last_sysfs_write, last_sysfs_write_len);
> +	else if (last_sysfs_write_len > 0)
> +		printk(KERN_EMERG "  written: %s\n", last_sysfs_write);
>  }
>  
>  /*
> @@ -200,12 +209,19 @@ flush_write_buffer(struct dentry * dentry, struct sysfs_buffer * buffer, size_t
>  	struct sysfs_dirent *attr_sd = dentry->d_fsdata;
>  	struct kobject *kobj = attr_sd->s_parent->s_dir.kobj;
>  	const struct sysfs_ops * ops = buffer->ops;
> +	int copylen = min(count, sizeof(last_sysfs_write) - 1);
>  	int rc;
>  
>  	/* need attr_sd for attr and ops, its parent for kobj */
>  	if (!sysfs_get_active(attr_sd))
>  		return -ENODEV;
>  
> +	while (copylen > 0 && buffer->page[copylen-1] == '\n')
> +		--copylen; /* never print trailing \n's */
> +	memcpy(last_sysfs_write, buffer->page, copylen);
> +	last_sysfs_write[copylen] = '\0';
> +	last_sysfs_write_len = count;
> +
>  	rc = ops->store(kobj, attr_sd->s_attr.attr, buffer->page, count);
>  
>  	sysfs_put_active(attr_sd);
> @@ -363,6 +379,9 @@ static int sysfs_open_file(struct inode *inode, struct file *file)
>  	if (file->f_mode & FMODE_WRITE) {
>  		if (!(inode->i_mode & S_IWUGO) || !ops->store)
>  			goto err_out;
> +		last_sysfs_write_len = 0;
> +	} else {
> +		last_sysfs_write_len = -1; /* magic value */

"magic" values are bad, please use a #define or something else that
makes more sense to those looking at the code in 5 years.

thanks,

greg k-h