Re: [RFC] persistent store (version 3) (part 1 of 2)

[Tony Luck <tony.luck@intel.com>]

On Tue, Dec 7, 2010 at 10:24 PM, Borislav Petkov <bp@alien8.de> 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