Messages in this thread | | | From | Seiji Aguchi <> | Subject | RE: [RFC][PATCH 2/2] write callback: Check if existing entry is erasable | Date | Wed, 4 Jul 2012 01:54:00 +0000 |
| |
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
| |