Hi Simon,
On Wed, May 13, 2020 at 10:24 PM Simon Glass <s...@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 <s...@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
