| Message ID | 20200513142359.147589-12-sjg@chromium.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | x86: cbfs: Various clean-ups to CBFS implementation | expand |
Hi Simon, On Wed, May 13, 2020 at 10:24 PM Simon Glass <sjg at chromium.org> wrote: > > This function currently returns a node pointer so there is no way to know > the error code. Also it uses data in BSS which seems unnecessary since the > caller might prefer to use a local variable. > > Update the function and split its body out into a separate function so we > can use it later. > > Signed-off-by: Simon Glass <sjg at chromium.org> > --- > > fs/cbfs/cbfs.c | 48 +++++++++++++++++++++++++++--------------------- > include/cbfs.h | 16 +++++++--------- > 2 files changed, 34 insertions(+), 30 deletions(-) > > diff --git a/fs/cbfs/cbfs.c b/fs/cbfs/cbfs.c > index 0db7cb9147..76613fa871 100644 > --- a/fs/cbfs/cbfs.c > +++ b/fs/cbfs/cbfs.c > @@ -371,40 +371,46 @@ const struct cbfs_cachenode *file_cbfs_find(const char *name) > return cbfs_find_file(&cbfs_s, name); > } > > -const struct cbfs_cachenode *file_cbfs_find_uncached(ulong end_of_rom, > - const char *name) > +static int find_uncached(struct cbfs_priv *priv, const char *name, u8 *start, This should be void *start > + struct cbfs_cachenode *node) > { > - struct cbfs_priv *priv = &cbfs_s; > - void *start; > - u32 size; > - u32 align; > - static struct cbfs_cachenode node; > - > - if (file_cbfs_load_header(priv, end_of_rom)) > - return NULL; > - > - start = priv->start; > - size = priv->header.rom_size; > - align = priv->header.align; > + int size = priv->header.rom_size; > + int align = priv->header.align; > > while (size >= align) { > - int ret; > int used; > + int ret; > > - ret = file_cbfs_next_file(priv, start, size, align, &node, > + ret = file_cbfs_next_file(priv, start, size, align, node, > &used); > if (ret == -ENOENT) > break; > else if (ret) > - return NULL; > - if (!strcmp(name, node.name)) > - return &node; > + return ret; > + if (!strcmp(name, node->name)) > + return 0; > > size -= used; > start += used; > } > - cbfs_s.result = CBFS_FILE_NOT_FOUND; > - return NULL; > + priv->result = CBFS_FILE_NOT_FOUND; > + > + return -ENOENT; > +} > + > +int file_cbfs_find_uncached(ulong end_of_rom, const char *name, > + struct cbfs_cachenode *node) > +{ > + struct cbfs_priv priv; > + u8 *start; > + int ret; > + > + ret = file_cbfs_load_header(&priv, end_of_rom); > + if (ret) > + return ret; > + start = (u8 *)(end_of_rom + 1 - priv.header.rom_size); This should be: start = priv->start; > + > + return find_uncached(&priv, name, start, node); > } > > const char *file_cbfs_name(const struct cbfs_cachenode *file) > diff --git a/include/cbfs.h b/include/cbfs.h > index 5cc27d682d..4dd3c0795d 100644 > --- a/include/cbfs.h > +++ b/include/cbfs.h > @@ -163,17 +163,15 @@ int cbfs_init_mem(ulong base, ulong size, struct cbfs_priv **privp); > /***************************************************************************/ > > /** > - * file_cbfs_find_uncached() - Find a file with a particular name in CBFS > - * without using the heap. > + * file_cbfs_find_uncached() - Find a file in CBFS without using the heap > * > - * @end_of_rom: Points to the end of the ROM the CBFS should be read > - * from. > - * @name: The name to search for. > - * > - * @return A handle to the file, or NULL on error. > + * @end_of_rom: Points to the end of the ROM the CBFS should be read from > + * @name: The name to search for > + * @node: Returns the node if found This is misleading. Based on the comments it seems that we should declare this to be: struct cbfs_cachenode **node > + * @return 0 on success, -ENOENT if not found, -EFAULT on bad header > */ > -const struct cbfs_cachenode *file_cbfs_find_uncached(ulong end_of_rom, > - const char *name); > +int file_cbfs_find_uncached(ulong end_of_rom, const char *name, > + struct cbfs_cachenode *node); > > /** > * file_cbfs_name() - Get the name of a file in CBFS. > -- Regards, Bin
diff --git a/fs/cbfs/cbfs.c b/fs/cbfs/cbfs.c index 0db7cb9147..76613fa871 100644 --- a/fs/cbfs/cbfs.c +++ b/fs/cbfs/cbfs.c @@ -371,40 +371,46 @@ const struct cbfs_cachenode *file_cbfs_find(const char *name) return cbfs_find_file(&cbfs_s, name); } -const struct cbfs_cachenode *file_cbfs_find_uncached(ulong end_of_rom, - const char *name) +static int find_uncached(struct cbfs_priv *priv, const char *name, u8 *start, + struct cbfs_cachenode *node) { - struct cbfs_priv *priv = &cbfs_s; - void *start; - u32 size; - u32 align; - static struct cbfs_cachenode node; - - if (file_cbfs_load_header(priv, end_of_rom)) - return NULL; - - start = priv->start; - size = priv->header.rom_size; - align = priv->header.align; + int size = priv->header.rom_size; + int align = priv->header.align; while (size >= align) { - int ret; int used; + int ret; - ret = file_cbfs_next_file(priv, start, size, align, &node, + ret = file_cbfs_next_file(priv, start, size, align, node, &used); if (ret == -ENOENT) break; else if (ret) - return NULL; - if (!strcmp(name, node.name)) - return &node; + return ret; + if (!strcmp(name, node->name)) + return 0; size -= used; start += used; } - cbfs_s.result = CBFS_FILE_NOT_FOUND; - return NULL; + priv->result = CBFS_FILE_NOT_FOUND; + + return -ENOENT; +} + +int file_cbfs_find_uncached(ulong end_of_rom, const char *name, + struct cbfs_cachenode *node) +{ + struct cbfs_priv priv; + u8 *start; + int ret; + + ret = file_cbfs_load_header(&priv, end_of_rom); + if (ret) + return ret; + start = (u8 *)(end_of_rom + 1 - priv.header.rom_size); + + return find_uncached(&priv, name, start, node); } const char *file_cbfs_name(const struct cbfs_cachenode *file) diff --git a/include/cbfs.h b/include/cbfs.h index 5cc27d682d..4dd3c0795d 100644 --- a/include/cbfs.h +++ b/include/cbfs.h @@ -163,17 +163,15 @@ int cbfs_init_mem(ulong base, ulong size, struct cbfs_priv **privp); /***************************************************************************/ /** - * file_cbfs_find_uncached() - Find a file with a particular name in CBFS - * without using the heap. + * file_cbfs_find_uncached() - Find a file in CBFS without using the heap * - * @end_of_rom: Points to the end of the ROM the CBFS should be read - * from. - * @name: The name to search for. - * - * @return A handle to the file, or NULL on error. + * @end_of_rom: Points to the end of the ROM the CBFS should be read from + * @name: The name to search for + * @node: Returns the node if found + * @return 0 on success, -ENOENT if not found, -EFAULT on bad header */ -const struct cbfs_cachenode *file_cbfs_find_uncached(ulong end_of_rom, - const char *name); +int file_cbfs_find_uncached(ulong end_of_rom, const char *name, + struct cbfs_cachenode *node); /** * file_cbfs_name() - Get the name of a file in CBFS.
This function currently returns a node pointer so there is no way to know the error code. Also it uses data in BSS which seems unnecessary since the caller might prefer to use a local variable. Update the function and split its body out into a separate function so we can use it later. Signed-off-by: Simon Glass <sjg at chromium.org> --- fs/cbfs/cbfs.c | 48 +++++++++++++++++++++++++++--------------------- include/cbfs.h | 16 +++++++--------- 2 files changed, 34 insertions(+), 30 deletions(-)