Messages in this thread | | | Date | Wed, 23 Oct 2002 18:40:20 +0100 | From | Christoph Hellwig <> | Subject | Re: [PATCH] LKCD for 2.5.44 (8/8): dump driver and build files |
| |
On Wed, Oct 23, 2002 at 02:45:07AM -0700, Matt D. Robinson wrote: > @@ -0,0 +1,26 @@ > +# > +# Makefile for the dump device drivers. > +# > +export-objs := dump_base.o > + > +obj-$(CONFIG_CRASH_DUMP) += dump.o > + > +dump-y := dump_base.o > +obj-$(CONFIG_CRASH_DUMP_BLOCKDEV) += dump_blockdev.o > +obj-$(CONFIG_CRASH_DUMP_COMPRESS_RLE) += dump_rle.o > +obj-$(CONFIG_CRASH_DUMP_COMPRESS_GZIP) += dump_gzip.o > +dump-objs += $(dump-y) > + > +ifeq ($(ARCH),i386) > +dump-objs += dump_i386.o > +endif > + > +ifeq ($(ARCH),alpha) > +dump-objs += dump_alpha.o > +endif > + > +ifeq ($(ARCH),ia64) > +dump-objs += dump_ia64.o > +endif
The makefile is a bit illogical. The dump-y stuff doesn't make much sense unless you actually uses it :) E.g.:
export-objs := dump_base.o
dump-y += dump_base.o dump-$(CONFIG_ALPHA) += dump_alpha.o dump-$(CONFIG_IA64) += dump_ia64.o dump-$(CONFIG_X86) += dump_i386.o dump-objs += $(dump-y)
obj-$(CONFIG_CRASH_DUMP) += dump.o obj-$(CONFIG_CRASH_DUMP_BLOCKDEV) += dump_blockdev.o obj-$(CONFIG_CRASH_DUMP_COMPRESS_RLE) += dump_rle.o obj-$(CONFIG_CRASH_DUMP_COMPRESS_GZIP) += dump_gzip.o
BTW, I can't see the alpha/ia64 stuff actually beeing included.
> +static kdev_t dump_device; /* the actual kdev_t device number */
should be dev_t, not kdev_t.
> +/* > + * Name: dump_read_proc() > + * Func: Read the proc data for dump tunables. > + */ > +static int > +dump_read_proc(char *page, char **start, off_t off, > + int count, int *eof, void *data) > +{ > + int len; > + char *out = page; > + struct proc_dir_entry *p = (struct proc_dir_entry *)data; > + > + > + if (0 == strcmp(p->name, DUMP_LEVEL_NAME)) { > + out += sprintf(out, "%d\n", dump_level); > + len = out - page; > + } else if (0 == strcmp(p->name, DUMP_FLAGS_NAME)) { > + out += sprintf(out, "%d\n", dump_flags); > + len = out - page; > + } else if (0 == strcmp(p->name, DUMP_COMPRESS_NAME)) { > + out += sprintf(out, "%d\n", dump_compress); > + len = out - page; > + } else if (0 == strcmp(p->name, DUMP_DEVICE_NAME)) { > + out += sprintf(out, "0x%x\n", kdev_val(dump_device)); > + len = out - page; > + } else { > + return 0; > + } > + len -= off; > + if (len < count) { > + *eof = 1; > + if (len <= 0) return 0; > + } else { > + len = count; > + } > + *start = page + off; > + return len;
Again: if you have one value per file use (ro) sysctl!
> +#ifndef UTSNAME_ENTRY_SZ > +#define UTSNAME_ENTRY_SZ 65 > +#endif
Shouldn't this be in a generic header?
> + dump_header_asm.dha_version = DUMP_ASM_VERSION_NUMBER; > + dump_header_asm.dha_header_size = sizeof(struct __dump_header_asm);
Indentation looks broken. Please run all sourcefiles through unexpand(1)
> + /* if this is the second call to this function, clean up ... */ > + if (dump_okay && !dump_page_buf_0) > + kfree((const void *)dump_page_buf_0);
kfree(NULL) is fine.. i.e.:
if (dump_okay) kfree(dump_page_buf_0);
Dito for all other places.
> +/* > + * Name: dump_release() > + * Func: Release the dump device -- it's never necessary to call > + * this function, but it's here regardless. > + */ > +static int > +dump_release(struct inode *i, struct file *f) > +{ > + return 0; > +}
Just remove it. You don't have to implement ->release.
> + > +/* > + * Name: dump_ioctl() > + * Func: Allow all dump tunables through a standard ioctl() mechanism. > + * This is far better than before, where we'd go through /proc, > + * because now this will work for multiple OS and architectures. > + */ > +static int > +dump_ioctl(struct inode *i, struct file *f, unsigned int cmd, unsigned long arg) > +{ > + /* check capabilities */ > + if (!capable(CAP_SYS_ADMIN)) > + return -EPERM; > + > + /* > + * This is the main mechanism for controlling get/set data > + * for various dump device parameters. The real trick here > + * is setting the dump device (DIOSDUMPDEV). That's what > + * triggers everything else. > + */ > + switch (cmd) { > + /* set dump_device */ > + case DIOSDUMPDEV: > + /* check flags */ > + if (!(f->f_flags & O_RDWR)) > + return -EPERM; > + > + __dump_open(); > + return (dump_open_kdev(to_kdev_t((dev_t)arg))); > + > + /* get dump_device */ > + case DIOGDUMPDEV: > + return (put_user((long)kdev_val(dump_device), (long *)arg)); > + > + /* set dump_level */ > + case DIOSDUMPLEVEL: > + /* check flags */ > + if (!(f->f_flags & O_RDWR)) > + return -EPERM; > + > + /* make sure we have a positive value */ > + if (arg < 0) > + return -EINVAL; > + dump_level = (int)arg; > + > + if (dump_level > DUMP_LEVEL_KERN) > + dump_unreserved_mem = 1; > + else > + dump_unreserved_mem = 0; > + > + if (dump_level > DUMP_LEVEL_USED) > + dump_unreferenced_mem = 1; > + else > + dump_unreferenced_mem = 0; > + > + if (dump_level > DUMP_LEVEL_ALL_RAM) > + dump_nonconventional_mem = 1; > + else > + dump_nonconventional_mem = 0; > + break; > + > + /* get dump_level */ > + case DIOGDUMPLEVEL: > + return (put_user((long)dump_level, (long *)arg)); > + > + /* set dump_flags */ > + case DIOSDUMPFLAGS: > + /* check flags */ > + if (!(f->f_flags & O_RDWR)) > + return -EPERM; > + > + /* make sure we have a positive value */ > + if (arg < 0) > + return -EINVAL; > + > + dump_flags = (int)arg; > + break; > + > + /* get dump_flags */ > + case DIOGDUMPFLAGS: > + return (put_user((long)dump_flags, (long *)arg)); > + > + /* set the dump_compress status */ > + case DIOSDUMPCOMPRESS: > + /* check flags */ > + if (!(f->f_flags & O_RDWR)) > + return -EPERM; > + > + return (dump_compress_init((int)arg)); > + > + /* get the dump_compress status */ > + case DIOGDUMPCOMPRESS: > + return (put_user((long)dump_compress, (long *)arg));
This is still the wrong interface :) At least the read/write one value stuff should be sysctl. For the rest ioctl is horribly ugly, but I'll leave the decision to Linus whether he wants to merge more ugly ioctl APIs.
> + > + /* try to create our dump device */ > + if (register_chrdev(CRASH_DUMP_MAJOR, "dump", &dump_fops)) { > + printk("cannot register dump character device!\n"); > + return -EBUSY; > + } > + > + /* initialize the dump headers to zero */ > + memset(&dump_header, 0, sizeof(dump_header)); > + memset(&dump_header_asm, 0, sizeof(dump_header_asm));
You don't have to zero structures lying in .bss
> + > + /* reset the dump function pointer */ > + dump_function_ptr = NULL;
pointless. the module memory will be vfree()ed anyway.
> +#ifdef MODULE > +MODULE_AUTHOR("Matt D. Robinson <yakker@sourceforge.net>"); > +MODULE_DESCRIPTION("Linux Kernel Crash Dump (LKCD) driver"); > +MODULE_LICENSE("GPL"); > +#endif /* MODULE */
No need for the ifdef - it'll be stubbed out anyway.
> +kdev_t dump_dev; /* the actual kdev_t device number */
> +int dump_blk_size; /* sector size for dump_device */ > +int dump_blk_shift; /* shift need to convert to sector size */ > +struct bio *dump_bio; /* bio structure for io request */ > +loff_t dump_offset; /* the offset in the output device */ > +unsigned long dumpdev_limit; /* the size limit of the block device */ > +int dump_io_abort = 0; /* set by end_io func during io error */ > + > +/* dump block device */ > +struct block_device *dump_bdev = NULL; > + > +/* indicates completion of each bio request */ > +volatile int bio_complete = 1;
Most of this should be static, right?
> + dump_bdev = bdget(kdev_t_to_nr(tmp_dump_device)); > + if (!dump_bdev) { > + retval = -ENODEV; > + goto err; > + } > + > + if (blkdev_get(dump_bdev, O_RDWR | O_LARGEFILE, 0, BDEV_RAW)) { > + retval = -ENODEV; > + goto err1; > + } > + > + if (!dump_page_buf) { > + retval = -EINVAL; > + goto err2; > + }
You need to call bd_claim to get exclusion on the bdev.
> +err3: dump_free_bio(); > +err2: if (dump_bdev) { > + blkdev_put(dump_bdev, BDEV_RAW); > + dump_bdev = NULL; > + } > +err1: if (dump_bdev) { > + bdput(dump_bdev); > + dump_bdev = NULL; > + } > +err: return retval; > +}
err3: dump_free_bio(); err2: if (dump_bdev) blkdev_put(dump_bdev, BDEV_RAW); goto err; err1: if (dump_bdev) bdput(dump_bdev); err: return retval;
> + > +static int __init > +dump_blockdev_init(void) > +{ > + dump_offset = 0;
once again, .bss is already per-zeroed :)
> +void > +__dump_cleanup(void) > +{ > + free_dha_stack(); > + return; > +}
I've noticed these superflous returns in void function a lot over the code. Linux kernel code usually avoids that.
> +static int __init > +dump_compress_rle_init(void) > +{ > + dump_register_compression(&dump_rle_compression); > + return 0;
Shouldn't there be some kind of error reporting? i.e.
return dump_register_compression(&dump_rle_compression); ?
> +/* header definitions for s390 dump */ > +#define DUMP_MAGIC_S390 0xa8190173618f23fdULL /* s390 magic number */ > +#define S390_DUMP_HEADER_SIZE 4096
should go to asm-s390/dump.h?
- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
| |