Messages in this thread | | | Date | Fri, 26 Oct 2018 13:35:14 -0700 | From | Joel Fernandes <> | Subject | Re: [RFC 1/6] pstore: map pstore types to names |
| |
On Fri, Oct 26, 2018 at 08:04:24PM +0100, Kees Cook wrote: > On Fri, Oct 26, 2018 at 7:00 PM, Joel Fernandes (Google) > <joel@joelfernandes.org> wrote: > > In later patches we will need to map types to names, so create a table > > for that which can also be used and reused in different parts of old and > > new code. Also use it to save the type in the PRZ which will be useful > > in later patches. > > Yes, I like it. :) Comments below...
I'm glad, thanks, my replies are below:
> > Signed-off-by: Joel Fernandes (Google) <joel@joelfernandes.org> > > --- > > fs/pstore/inode.c | 44 ++++++++++++++++++++------------------ > > fs/pstore/ram.c | 4 +++- > > include/linux/pstore.h | 29 +++++++++++++++++++++++++ > > include/linux/pstore_ram.h | 2 ++ > > 4 files changed, 57 insertions(+), 22 deletions(-) > > > > diff --git a/fs/pstore/inode.c b/fs/pstore/inode.c > > index 5fcb845b9fec..43757049d384 100644 > > --- a/fs/pstore/inode.c > > +++ b/fs/pstore/inode.c > > @@ -304,6 +304,7 @@ int pstore_mkfile(struct dentry *root, struct pstore_record *record) > > struct dentry *dentry; > > struct inode *inode; > > int rc = 0; > > + enum pstore_type_id type; > > char name[PSTORE_NAMELEN]; > > struct pstore_private *private, *pos; > > unsigned long flags; > > @@ -335,43 +336,44 @@ int pstore_mkfile(struct dentry *root, struct pstore_record *record) > > goto fail_alloc; > > private->record = record; > > > > - switch (record->type) { > > + type = record->type; > > Let's rename PSTORE_TYPE_UNKNOWN in the enum to be PSTORE_TYPE_MAX and > != 255 (just leave it at the end). The value is never exposed to > userspace (nor to backend storage), so we can instead use it as the > bounds-check for doing type -> name mappings. (The one use in erst can > just be renamed.) > > Then we can add a function to do the bounds checking and mapping > (instead of using a bare array lookup). > > > + switch (type) { > > case PSTORE_TYPE_DMESG: > > - scnprintf(name, sizeof(name), "dmesg-%s-%llu%s", > > - record->psi->name, record->id, > > - record->compressed ? ".enc.z" : ""); > > + scnprintf(name, sizeof(name), "%s-%s-%llu%s", > > + pstore_names[type], record->psi->name, record->id, > > + record->compressed ? ".enc.z" : ""); > > break; > > case PSTORE_TYPE_CONSOLE: > > - scnprintf(name, sizeof(name), "console-%s-%llu", > > - record->psi->name, record->id); > > + scnprintf(name, sizeof(name), "%s-%s-%llu", > > + pstore_names[type], record->psi->name, record->id); > > break; > > case PSTORE_TYPE_FTRACE: > > - scnprintf(name, sizeof(name), "ftrace-%s-%llu", > > - record->psi->name, record->id); > > + scnprintf(name, sizeof(name), "%s-%s-%llu", > > + pstore_names[type], record->psi->name, record->id); > > break; > > case PSTORE_TYPE_MCE: > > - scnprintf(name, sizeof(name), "mce-%s-%llu", > > - record->psi->name, record->id); > > + scnprintf(name, sizeof(name), "%s-%s-%llu", > > + pstore_names[type], record->psi->name, record->id); > > break; > > case PSTORE_TYPE_PPC_RTAS: > > - scnprintf(name, sizeof(name), "rtas-%s-%llu", > > - record->psi->name, record->id); > > + scnprintf(name, sizeof(name), "%s-%s-%llu", > > + pstore_names[type], record->psi->name, record->id); > > break; > > case PSTORE_TYPE_PPC_OF: > > - scnprintf(name, sizeof(name), "powerpc-ofw-%s-%llu", > > - record->psi->name, record->id); > > + scnprintf(name, sizeof(name), "%s-%s-%llu", > > + pstore_names[type], record->psi->name, record->id); > > break; > > case PSTORE_TYPE_PPC_COMMON: > > - scnprintf(name, sizeof(name), "powerpc-common-%s-%llu", > > - record->psi->name, record->id); > > + scnprintf(name, sizeof(name), "%s-%s-%llu", > > + pstore_names[type], record->psi->name, record->id); > > break; > > case PSTORE_TYPE_PMSG: > > - scnprintf(name, sizeof(name), "pmsg-%s-%llu", > > - record->psi->name, record->id); > > + scnprintf(name, sizeof(name), "%s-%s-%llu", > > + pstore_names[type], record->psi->name, record->id); > > break; > > case PSTORE_TYPE_PPC_OPAL: > > - scnprintf(name, sizeof(name), "powerpc-opal-%s-%llu", > > - record->psi->name, record->id); > > + scnprintf(name, sizeof(name), "%s-%s-%llu", > > + pstore_names[type], record->psi->name, record->id); > > break; > > case PSTORE_TYPE_UNKNOWN: > > scnprintf(name, sizeof(name), "unknown-%s-%llu", > > All of these, including PSTORE_TYPE_UNKNOWN are now identical (you can > include the .enc.z logic in for all of them. I think the entire switch > can be dropped, yes? > > scnprintf(name, sizeof(name), "%s-%s-%llu%s", > pstore_name(record->type), record->psi->name, record->id, > record->compressed ? ".enc.z" : "")
True! I'll just do that. Sounds good! The PSTORE_TYPE_UNKNOWN and the below "default:" case could be combined I guessed. Its a great idea and lesser lines, thanks!
> > dump_mem_sz, cxt->record_size, > > &cxt->max_dump_cnt, 0, 0); > > if (err) > > diff --git a/include/linux/pstore.h b/include/linux/pstore.h > > index a15bc4d48752..4a3dbdffd8d3 100644 > > --- a/include/linux/pstore.h > > +++ b/include/linux/pstore.h > > @@ -47,6 +47,21 @@ enum pstore_type_id { > > PSTORE_TYPE_UNKNOWN = 255 > > }; > > > > +/* names should be in the same order as the above table */ > > +static char __maybe_unused *pstore_names[] = { > > This should be static const char * const pstore_names[], I think?
Sure, I'll add the const(s) to it.
> > + "dmesg", > > + "mce", > > + "console", > > + "ftrace", > > + "rtas", > > + "powerpc-ofw", > > + "powerpc-common", > > + "pmsg", > > + "powerpc-opal", > > + "event", > > + "unknown", /* unknown should be the last string */ > > End this with a NULL for a cheaper compare below.
Agreed, I am wondering if we need the unknown string though if we terminate the type enum table with a MAX. I'll look more into that.
> > +}; > > + > > struct pstore_info; > > /** > > * struct pstore_record - details of a pstore record entry > > @@ -274,4 +289,18 @@ pstore_ftrace_write_timestamp(struct pstore_ftrace_record *rec, u64 val) > > } > > #endif > > > > +static inline enum pstore_type_id pstore_name_to_type(const char *name) > > +{ > > + char *str; > > + int i = 0; > > + > > + for (; strncmp(pstore_names[i], "unknown", 7); i++) { > char **ptr; > > for (ptr = pstores_names; *ptr; ptr++) { > > > + str = pstore_names[i]; > > + if (!strncmp(name, str, strlen(str))) > > "n" version not needed: the LHS is controlled and we want full matching: > > if (!strcmp(*ptr, name))
Sounds good, but now that we are adding PSTORE_TYPE_MAX, I think it would be cleaner and safer if I iterated until PSTORE_TYPE_MAX, so I'll use that to terminate the loop?
> > + return (enum pstore_type_id)i; > > I don't think this cast is needed? > > return (ptr - pstores_names);
But I have the index variable, so it would be cleaner to just return that? I believe I can just drop the cast and do that.
thanks,
- Joel
| |