lkml.org 
[lkml]   [2012]   [Jul]   [3]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
SubjectRE: [RFC][PATCH 2/2] write callback: Check if existing entry is erasable
Date
Thank you for reviewing my patch.

> > + while (1) {
> > + prev_why = pstore_get_reason_str(prev_reason);
> > + if (!strncmp(entry->var.Data, prev_why, strlen(prev_why)))
> > + break;
> > + prev_reason++;
> > + }
>
> This loop should probably be hardened to cope with bad data. The patch description and code below show intention to overwrite
> garbage data, but this loop will never exit on garbage data.

OK. I will remove infinite loop above.

> > + switch (prev_reason) {
> > + case KMSG_DUMP_PANIC:
> > + case KMSG_DUMP_EMERG:
> > + /* Never erase panic or emergency message */
> > + break;
> > + case KMSG_DUMP_OOPS:
> > + case KMSG_DUMP_RESTART:
> > + case KMSG_DUMP_HALT:
> > + case KMSG_DUMP_POWEROFF:
> > + /* Can erase if new one is error message */
> > + if (new_reason <= KMSG_DUMP_EMERG)
> > + is_erasable = 1;
> > + break;
> > + default:
> > + /* Can erase unknown message */
> > + is_erasable = 1;
>
> It is probably safer to actually complain here at compile time if a reason is missing. prev_reason is an enum though, so if all cases are
> accounted for and no default is specified, gcc should complain if a new KMSG_DUMP enum value is added to the codebase without
> considering this logic here.

It makes sense.
I will remove "default" and add all cases of "KMSG_DUMP_*".
In latest linus-tree, KMSG_DUMP_UNDEF is added.
I will check how we should treat it.


> > + if (!can_erase_entry(entry, reason)) {
> > + /* return without writing new entry */
> > + spin_unlock(&efivars->lock);
> > + *id = part;
> > + return ret;
> > + }
>
> I'm not sure how comfortable I am with the code duplication here. Was using a flag not workable?

If efi_pstore doesn't write new entry, it should return with non-zero value.
I will update my patch by changing to "return -EEXIST " or so.

Also, I will fix typo you pointed out.

Thanks,

Seiji


\
 
 \ /
  Last update: 2012-07-04 04:21    [W:0.093 / U:0.052 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site