Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Date:   Fri, 26 Oct 2018 12:36:37 -0700
From:   Joel Fernandes <joel@joelfernandes.org>
To:     Kees Cook <keescook@chromium.org>
Cc:     LKML <linux-kernel@vger.kernel.org>, kernel-team@android.com,
        Anton Vorontsov <anton@enomsg.org>,
        Colin Cross <ccross@android.com>,
        Tony Luck <tony.luck@intel.com>
Subject: Re: [RFC 4/6] pstore: further reduce ramoops_get_next_prz arguments
 by passing record
Message-ID: <20181026193637.GA122104@joelaf.mtv.corp.google.com>
References: <20181026180042.52199-1-joel@joelfernandes.org>
 <20181026180042.52199-4-joel@joelfernandes.org>
 <CAGXu5jJi5M+wfF8X7ud2jTjdJZRE4sCnZgv9OA9ZX-wwpZ5PnA@mail.gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <CAGXu5jJi5M+wfF8X7ud2jTjdJZRE4sCnZgv9OA9ZX-wwpZ5PnA@mail.gmail.com>
User-Agent: Mutt/1.10.1 (2018-07-13)
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

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@joelfernandes.org> 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@joelfernandes.org>
> > ---
> >  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