Messages in this thread | | | From | Kees Cook <> | Date | Fri, 26 Oct 2018 20:27:49 +0100 | Subject | Re: [RFC 3/6] pstore: remove max argument from ramoops_get_next_prz |
| |
On Fri, Oct 26, 2018 at 8:22 PM, Joel Fernandes <joel@joelfernandes.org> wrote: > On Fri, Oct 26, 2018 at 11:00:39AM -0700, Joel Fernandes (Google) wrote: >> From the code flow, the 'max' checks are already being done on the prz >> passed to ramoops_get_next_prz. Lets remove it to simplify this function >> and reduce its arguments. >> >> Signed-off-by: Joel Fernandes (Google) <joel@joelfernandes.org> >> --- >> fs/pstore/ram.c | 14 ++++++-------- >> 1 file changed, 6 insertions(+), 8 deletions(-) >> >> diff --git a/fs/pstore/ram.c b/fs/pstore/ram.c >> index cbfdf4b8e89d..3055e05acab1 100644 >> --- a/fs/pstore/ram.c >> +++ b/fs/pstore/ram.c >> @@ -124,14 +124,14 @@ static int ramoops_pstore_open(struct pstore_info *psi) >> } >> >> static struct persistent_ram_zone * >> -ramoops_get_next_prz(struct persistent_ram_zone *przs[], uint *c, uint max, >> +ramoops_get_next_prz(struct persistent_ram_zone *przs[], uint *c, >> u64 *id, enum pstore_type_id *typep, bool update) >> { >> struct persistent_ram_zone *prz; >> int i = (*c)++; >> >> /* Give up if we never existed or have hit the end. */ >> - if (!przs || i >= max) >> + if (!przs) >> return NULL; >> >> prz = przs[i]; > > Ah, looks like I may have introduced an issue here since 'i' isn't checked by > the caller for the single prz case, its only checked for the multiple prz > cases, so something like below could be folded in. I still feel its better > than passing the max argument. > > Another thought is, even better we could have a different function when > there's only one prz and not have to pass an array, just pass the first > element? Something like... > > ramoops_get_next_prz_single(struct persistent_ram_zone *prz, uint *c, > enum pstore_type_id *typep, bool update) > And for the _single case, we also wouldn't need to pass id so that's another > argument less. > > Let me know what you think, otherwise something like the below will need to > be folded in to fix this patch... thanks. > > ----8<--- > > diff --git a/fs/pstore/ram.c b/fs/pstore/ram.c > index 5702b692bdb9..061d2af2485b 100644 > --- a/fs/pstore/ram.c > +++ b/fs/pstore/ram.c > @@ -268,17 +268,19 @@ static ssize_t ramoops_pstore_read(struct pstore_record *record) > } > } > > - if (!prz_ok(prz)) > + if (!prz_ok(prz) && !cxt->console_read_cnt) { > prz = ramoops_get_next_prz(&cxt->cprz, &cxt->console_read_cnt, > record, 0); > + } > > - if (!prz_ok(prz)) > + if (!prz_ok(prz) && !cxt->pmsg_read_cnt) > prz = ramoops_get_next_prz(&cxt->mprz, &cxt->pmsg_read_cnt, > record, 0); > > /* ftrace is last since it may want to dynamically allocate memory. */ > if (!prz_ok(prz)) { > - if (!(cxt->flags & RAMOOPS_FLAG_FTRACE_PER_CPU)) { > + if (!(cxt->flags & RAMOOPS_FLAG_FTRACE_PER_CPU) && > + !cxt->ftrace_read_cnt) { > prz = ramoops_get_next_prz(cxt->fprzs, > &cxt->ftrace_read_cnt, record, 0); > } else {
Ah yeah, good catch! I think your added fix is right. I was pondering asking you to remove the & on the *_read_cnt and having the caller do the increment:
while (cxt->dump_read_cnt < cxt->max_dump_cnt && !prz) { prz = ramoops_get_next_prz(cxt->dprzs, cxt->dump_read_cnt++, &record->id, &record->type, PSTORE_TYPE_DMESG, 1);
-Kees
-- Kees Cook
| |