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

2018-10-26 Thread 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)
>  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) 
> > ---
> >  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, >dump_read_cnt,
> > -  >id, >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

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

2018-10-26 Thread 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)
>  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) 
> > ---
> >  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, >dump_read_cnt,
> > -  >id, >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

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

2018-10-26 Thread Kees Cook 
On Fri, Oct 26, 2018 at 7:00 PM, Joel Fernandes (Google)
 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) 
> ---
>  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, >dump_read_cnt,
> -  >id, >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(>cprz, >console_read_cnt,
> -  >id, >type, 0);
> +  record, 0);
>
> if (!prz_ok(prz))
> prz = ramoops_get_next_prz(>mprz, >pmsg_read_cnt,
> -  >id, >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,
> -   >ftrace_read_cnt, >id,
> -   >type, 0);
> +   >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,
> >ftrace_read_cnt,
> -   >id,
> -   >type, 0);
> +   record, 0);
>
> if (!prz_ok(prz_next))
> continue;
> --
> 2.19.1.568.g152ad8e336-goog
>



-- 
Kees Cook

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

2018-10-26 Thread Kees Cook 
On Fri, Oct 26, 2018 at 7:00 PM, Joel Fernandes (Google)
 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) 
> ---
>  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, >dump_read_cnt,
> -  >id, >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(>cprz, >console_read_cnt,
> -  >id, >type, 0);
> +  record, 0);
>
> if (!prz_ok(prz))
> prz = ramoops_get_next_prz(>mprz, >pmsg_read_cnt,
> -  >id, >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,
> -   >ftrace_read_cnt, >id,
> -   >type, 0);
> +   >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,
> >ftrace_read_cnt,
> -   >id,
> -   >type, 0);
> +   record, 0);
>
> if (!prz_ok(prz_next))
> continue;
> --
> 2.19.1.568.g152ad8e336-goog
>



-- 
Kees Cook

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

2018-10-26 Thread Joel Fernandes (Google) 
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) 
---
 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;
 
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, >dump_read_cnt,
-  >id, >type, 1);
+  record, 1);
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(>cprz, >console_read_cnt,
-  >id, >type, 0);
+  record, 0);
 
if (!prz_ok(prz))
prz = ramoops_get_next_prz(>mprz, >pmsg_read_cnt,
-  >id, >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,
-   >ftrace_read_cnt, >id,
-   >type, 0);
+   >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,
>ftrace_read_cnt,
-   >id,
-   >type, 0);
+   record, 0);
 
if (!prz_ok(prz_next))
continue;
-- 
2.19.1.568.g152ad8e336-goog

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

2018-10-26 Thread Joel Fernandes (Google) 
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) 
---
 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;
 
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, >dump_read_cnt,
-  >id, >type, 1);
+  record, 1);
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(>cprz, >console_read_cnt,
-  >id, >type, 0);
+  record, 0);
 
if (!prz_ok(prz))
prz = ramoops_get_next_prz(>mprz, >pmsg_read_cnt,
-  >id, >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,
-   >ftrace_read_cnt, >id,
-   >type, 0);
+   >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,
>ftrace_read_cnt,
-   >id,
-   >type, 0);
+   record, 0);
 
if (!prz_ok(prz_next))
continue;
-- 
2.19.1.568.g152ad8e336-goog