Re: [RFC 4/6] pstore: further reduce ramoops_get_next_prz arguments by passing record

[Joel Fernandes]

On Fri, Oct 26, 2018 at 08:32:16PM +0100, Kees Cook wrote:
> On Fri, Oct 26, 2018 at 7:00 PM, Joel Fernandes (Google)
> <joel@xxxxxxxxxxxxxxxxx> wrote:
> > Both the id and type fields of a pstore_record are set by
> > ramoops_get_next_prz. So we can just pass a pointer to the pstore_record
> > instead of passing individual elements. This results in cleaner more
> > readable code and fewer lines.
> >
> > Signed-off-by: Joel Fernandes (Google) <joel@xxxxxxxxxxxxxxxxx>
> > ---
> >  fs/pstore/ram.c | 18 ++++++++----------
> >  1 file changed, 8 insertions(+), 10 deletions(-)
> >
> > diff --git a/fs/pstore/ram.c b/fs/pstore/ram.c
> > index 3055e05acab1..710c3d30bac0 100644
> > --- a/fs/pstore/ram.c
> > +++ b/fs/pstore/ram.c
> > @@ -125,7 +125,7 @@ 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,
> > -                    u64 *id, enum pstore_type_id *typep, bool update)
> > +                    struct pstore_record *record, bool update)
> >  {
> >         struct persistent_ram_zone *prz;
> >         int i = (*c)++;
> > @@ -145,8 +145,8 @@ ramoops_get_next_prz(struct persistent_ram_zone *przs[], uint *c,
> >         if (!persistent_ram_old_size(prz))
> >                 return NULL;
> >
> > -       *typep = prz->type;
> > -       *id = i;
> > +       record->type = prz->type;
> > +       record->id = i;
> 
> Yes yes. I've been meaning to get all this cleaned up after I
> refactored everything to actually HAVE record at all. :P
> 
> >
> >         return prz;
> >  }
> > @@ -254,7 +254,7 @@ static ssize_t ramoops_pstore_read(struct pstore_record *record)
> >         /* Find the next valid persistent_ram_zone for DMESG */
> >         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, 1);
> > +                                          record, 1);
> 
> In another patch, I think you could drop the "update" field too, and
> use the record->type instead to determine if update is needed. Like:
> 
> static struct persistent_ram_zone *
> ramoops_get_next_prz(struct persistent_ram_zone *przs[], uint c,
>                                       struct pstore_record *record)
> {
>     bool update = (record->type == PSTORE_TYPE_DMESG);
> ...

Yes, I agree, I'll do that :)

thanks!

 - Joel