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;
MIME-Version: 1.0
In-Reply-To: <20181026180042.52199-4-joel@joelfernandes.org>
References: <20181026180042.52199-1-joel@joelfernandes.org> <20181026180042.52199-4-joel@joelfernandes.org>
From:   Kees Cook <keescook@chromium.org>
Date:   Fri, 26 Oct 2018 20:32:16 +0100
Message-ID: <CAGXu5jJi5M+wfF8X7ud2jTjdJZRE4sCnZgv9OA9ZX-wwpZ5PnA@mail.gmail.com>
Subject: Re: [RFC 4/6] pstore: further reduce ramoops_get_next_prz arguments
 by passing record
To:     "Joel Fernandes (Google)" <joel@joelfernandes.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>
Content-Type: text/plain; charset="UTF-8"
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

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);
...

>                 if (!prz_ok(prz))
>                         continue;
>                 header_length = ramoops_read_kmsg_hdr(persistent_ram_old(prz),
> @@ -270,18 +270,17 @@ static ssize_t ramoops_pstore_read(struct pstore_record *record)
>
>         if (!prz_ok(prz))
>                 prz = ramoops_get_next_prz(&cxt->cprz, &cxt->console_read_cnt,
> -                                          &record->id, &record->type, 0);
> +                                          record, 0);
>
>         if (!prz_ok(prz))
>                 prz = ramoops_get_next_prz(&cxt->mprz, &cxt->pmsg_read_cnt,
> -                                          &record->id, &record->type, 0);
> +                                          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)) {
>                         prz = ramoops_get_next_prz(cxt->fprzs,
> -                                       &cxt->ftrace_read_cnt, &record->id,
> -                                       &record->type, 0);
> +                                       &cxt->ftrace_read_cnt, record, 0);
>                 } else {
>                         /*
>                          * Build a new dummy record which combines all the
> @@ -298,8 +297,7 @@ static ssize_t ramoops_pstore_read(struct pstore_record *record)
>                         while (cxt->ftrace_read_cnt < cxt->max_ftrace_cnt) {
>                                 prz_next = ramoops_get_next_prz(cxt->fprzs,
>                                                 &cxt->ftrace_read_cnt,
> -                                               &record->id,
> -                                               &record->type, 0);
> +                                               record, 0);
>
>                                 if (!prz_ok(prz_next))
>                                         continue;
> --
> 2.19.1.568.g152ad8e336-goog
>


-- 
Kees Cook