[prev in list] [next in list] [prev in thread] [next in thread] 

List:       osdl-fastboot
Subject:    [Fastboot] Re: [PATCH 4/4] Kexec based crash dumps
From:       Andrew Morton <akpm () osdl ! org>
Date:       2004-07-28 20:19:44
Message-ID: 20040728131944.728f10e9.akpm () osdl ! org
[Download RAW message or body]

Hariprasad Nellitheertha <hari@in.ibm.com> wrote:
>
> This patch contains the code to abstract the dump in the form of an ELF 
> format file.
> 

> diff -Naurp before/fs/proc/kcore.c after/fs/proc/kcore.c
> --- before/fs/proc/kcore.c	2004-06-16 10:49:31.000000000 +0530
> +++ after/fs/proc/kcore.c	2004-07-28 07:25:25.000000000 +0530
> @@ -21,6 +21,7 @@
>  #include <asm/uaccess.h>
>  #include <asm/io.h>
>  
> +void elf_kcore_store_hdr(char *, int, int);

This prototype addition seems unnecessary.
 
> +#include <linux/crash_dump.h>
>  #include <asm/uaccess.h>
>  #include <asm/pgtable.h>
>  #include <asm/io.h>
> @@ -53,6 +55,8 @@
>  
>  #define LOAD_INT(x) ((x) >> FSHIFT)
>  #define LOAD_FRAC(x) LOAD_INT(((x) & (FIXED_1-1)) * 100)
> +extern unsigned int dump_enabled;
> +extern unsigned long saved_max_pfn;

Please put extern decls in header files, not in .c files.  Somewhere where
the definition and all users see the same declaration.


>  /*
>   * Warning: stuff below (imported functions) assumes that its output will fit
>   * into one page. For some of those functions it may be wrong. Moreover, we
> @@ -642,6 +646,7 @@ static struct file_operations proc_sysrq
>  #endif
>  
>  struct proc_dir_entry *proc_root_kcore;
> +struct proc_dir_entry *proc_vmcore;

Can this have static scope?

>  
>  static void create_seq_entry(char *name, mode_t mode, struct file_operations *f)
>  {
> @@ -715,6 +720,17 @@ void __init proc_misc_init(void)
>  				(size_t)high_memory - PAGE_OFFSET + PAGE_SIZE;
>  	}
>  #endif
> +#ifdef CONFIG_CRASH_DUMP
> +	if(dump_enabled) {

One space after the `if'.

> +static int open_vmcore(struct inode * inode, struct file * filp)
> +{
> +	return capable(CAP_SYS_RAWIO) ? 0 : -EPERM;
> +}

/proc file permissions should be sufficient.

> +
> +static rwlock_t kcp_lock = RW_LOCK_UNLOCKED;

This needs a comment describing what it is locking.

> +extern char saved_command_line[];

This is already declared in include/linux/init.h

> +/*
> + * Reads from the hmem device from given offset till
> + * given count
> + */
> +static ssize_t read_from_hmem(char *buf, size_t count,
> +			     loff_t *ppos, int userbuf)
> +{
> +	unsigned long pfn, p = *ppos;
> +
> +	if (p == -1) {
> +		pfn = -1;
> +		p = 0;
> +	} else {	
> +	        pfn = p / PAGE_SIZE;
> +	        p = p % PAGE_SIZE;
> +	}
> +        if ((pfn != -1) && (pfn > saved_max_pfn))
> +                return -EINVAL;
> +
> +        if (count > PAGE_SIZE - p)
> +                count = PAGE_SIZE - p;
> +
> +	if (copy_hmem_page(pfn, buf, count, userbuf))
> +		return -EFAULT;
> +
> +        *ppos += count;
> +        return 0;
> +}

whitespace is weird here.

> +
> +/*
> + * store an ELF crash dump header in the supplied buffer
> + * nphdr is the number of elf_phdr to insert
> + */
> +static void elf_vmcore_store_hdr(char *bufp, int nphdr, int dataoff)
> +{
> +	struct elf_prstatus prstatus;	/* NT_PRSTATUS */
> +	struct memelfnote notes[1];
> +	char reg_buf[PAGE_SIZE];
> +	loff_t reg_ppos;
> +	char *buf = bufp;

No, we cannot allocate a PAGE_SIZE local.  It'll instantly explode on 4k
stacks.

> +			
> +		if (read_from_hmem(buffer, tsz, (loff_t *)&p, 1))
> +			return -EINVAL;

hm.  I prefer that functions only have a single `return' point, please.




_______________________________________________
fastboot mailing list
fastboot@lists.osdl.org
http://lists.osdl.org/mailman/listinfo/fastboot


[prev in list] [next in list] [prev in thread] [next in thread]