On Tue, Dec 7, 2010 at 10:24 PM, Borislav Petkov <bp@xxxxxxxxx> wrote:
> looks good. Minor nitpicks below.
Thanks for looking at this.
>> 1) Is "struct file" too big to be on the stack? I can change it to kmalloc() it.
>
> Well, this happens during normal operation when we init the pstore and
> since we don't need it after pstore_mkwrite has returned (do we?), I
> guess we should be fine.
struct file is 184 bytes (with the CONFIG options I am using, it can get
a bit larger, but not much).
>> + the dying moments. In the case of a panic() the last part
>
> "panic" (I'd remove the
> brackets)
Ok. Will do.
>> + dentry = d_alloc_name(root, name);
>> + if (!IS_ERR(dentry))
>> + d_add(dentry, inode);
>
> what happens if it IS_ERR? Error handling like
>
> goto d_alloc_error;
>
> d_alloc_error:
> iput(inode);
> return -ENOSPC;
>
>
> or similar, at least this is what ramfs seems to be doing.
Good point - I need to clean up if things fail (and akpm would
like to see me re-factor so that there is only one "return"
statement for better maintainabiliy). I'll fix up stuff here.
>> + kmsg_dump_register(&pstore_dumper);
>
> You don't check psi->write() method's existence anymore, I'm assuming
> this is implied now... ?
Andrew's comments on version 2 were:
>It doesn't seem appropriate to check this here. It's a programming
>error! Just install the thing and let the kernel oops - the programmer
>will notice.
So I dropped the tests ... just checking whether the function pointer
was NULL wasn't a very strong test anyway.
>> + if (!psinfo)
>> + return -ENODEV;
>
> newline.
OK.
>> +#define PSTOREFS_MAGIC 0x6165676C
>
> what does that mean anyway? "aegl" :)
My initials: Anthony Eric George Luck. I've been using "aegl"
as my Unix login since 1979 (6th Edition on a pdp11/34).
>> +/* types */
>> +#define PSTORE_TYPE_DMESG 0
>> +#define PSTORE_TYPE_MCE 1
>
> You could make this into a proper enum
>
> enum pstore_type_id {
> PSTORE_TYPE_DMESG = 0,
> PSTORE_TYPE_MCE = 1,
> PSTORE_TYPE_MAX,
> };
OK. Type checking is nice. I think I might need a
PSTORE_TYPE_UNKNOWN to handle the case
where a new platform driver saves a record to
persistent store, and then the user boots an older
kernel that doesn't know about the new type (in
the APEI/ERST driver the type turns into a UUID
in the UEFI header for the record - so there is some
mapping going on at that level).
>> +struct pstore_info {
>> + struct module *owner;
>> + char *name;
>> + struct mutex mutex; /* serialize access to 'buf' */
>
> [ maybe a more descriptive variable name like buf_mutex or whatever ]
Sure. Will change.
>> +#if defined(CONFIG_PSTORE) || defined(CONFIG_PSTORE_MODULE)
>
> What is CONFIG_PSTORE_MODULE? Can't seem to find it in your (2 of 2)
> message either.
This is a remnant of when PSTORE was tristate - when I chose 'm'
rather than 'y' in "make menuconfig" I ended up with CONFIG_PSTORE_MODULE
rather than CONFIG_PSTORE. But now it is just a "bool" this isn't
needed any more. Will fix.
-Tony
--
To unsubscribe from this list: send the line "unsubscribe linux-arch" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
