[PATCH 11/13] cbfs: Change file_cbfs_find_uncached() to return an error

[Bin Meng]

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