[PATCH v3] cmd: cat: add new command

[Simon Glass]

Hi,

On Thu, 18 Aug 2022 at 11:08, Heinrich Schuchardt <xypron.glpk at gmx.de> wrote:
>
> On 8/18/22 18:54, Roger Knecht wrote:
> > Add cat command to print file content to standard out
> >
> > Signed-off-by: Roger Knecht <rknecht at pm.me>
> > ---
> > v3:
> >   - Disable 'cat' by default (CONFIG_CMD_CAT=n)
> >   - Enable 'cat' in sandbox and sandbox64 defconfig
> >   - Use map_to_sysmem() to fix "phys_to_virt: Cannot map sandbox address"
> >   - Use puts() instead of a loop
> >   - Added python test
> >   - Addd usage documentation
> >
> > v2:
> >   - Moved cat from boot to shell commands
> >   - Added MAINTAINERS entry
> >   - Added comments
> >   - Improved variable naming
> >
> >   MAINTAINERS                        |  5 +++
> >   cmd/Kconfig                        |  6 +++
> >   cmd/Makefile                       |  1 +
> >   cmd/cat.c                          | 67 ++++++++++++++++++++++++++++++
> >   configs/sandbox64_defconfig        |  1 +
> >   configs/sandbox_defconfig          |  1 +
> >   doc/usage/cmd/cat.rst              | 49 ++++++++++++++++++++++
> >   test/py/tests/test_cat/conftest.py | 33 +++++++++++++++
> >   test/py/tests/test_cat/test_cat.py | 22 ++++++++++
> >   9 files changed, 185 insertions(+)
> >   create mode 100644 cmd/cat.c
> >   create mode 100644 doc/usage/cmd/cat.rst
> >   create mode 100644 test/py/tests/test_cat/conftest.py
> >   create mode 100644 test/py/tests/test_cat/test_cat.py

Very nice patch, could be a good example for others to use.

> >
> > diff --git a/MAINTAINERS b/MAINTAINERS
> > index 5857fbf398..2864f84274 100644
> > --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> > @@ -765,6 +765,11 @@ M:       Simon Glass <sjg at chromium.org>
> >   S:  Maintained
> >   F:  tools/buildman/
> >
> > +CAT
> > +M:   Roger Knecht <rknecht at pm.me>
> > +S:   Maintained
> > +F:   cmd/cat.c
> > +
> >   CFI FLASH
> >   M:  Stefan Roese <sr at denx.de>
> >   S:  Maintained
> > diff --git a/cmd/Kconfig b/cmd/Kconfig
> > index 211ebe9c87..ce7e876475 100644
> > --- a/cmd/Kconfig
> > +++ b/cmd/Kconfig
> > @@ -1531,6 +1531,12 @@ endmenu
> >
> >   menu "Shell scripting commands"
> >
> > +config CMD_CAT
> > +     bool "cat"
> > +     default n

Not needed as things default to n

> > +     help
> > +       Print file to standard output
> > +
> >   config CMD_ECHO
> >       bool "echo"
> >       default y
> > diff --git a/cmd/Makefile b/cmd/Makefile
> > index 6e87522b62..1d2590e958 100644
> > --- a/cmd/Makefile
> > +++ b/cmd/Makefile
> > @@ -38,6 +38,7 @@ obj-$(CONFIG_CMD_BOOTZ) += bootz.o
> >   obj-$(CONFIG_CMD_BOOTI) += booti.o
> >   obj-$(CONFIG_CMD_BTRFS) += btrfs.o
> >   obj-$(CONFIG_CMD_BUTTON) += button.o
> > +obj-$(CONFIG_CMD_CAT) += cat.o
> >   obj-$(CONFIG_CMD_CACHE) += cache.o
> >   obj-$(CONFIG_CMD_CBFS) += cbfs.o
> >   obj-$(CONFIG_CMD_CLK) += clk.o
> > diff --git a/cmd/cat.c b/cmd/cat.c
> > new file mode 100644
> > index 0000000000..c217617cd6
> > --- /dev/null
> > +++ b/cmd/cat.c
> > @@ -0,0 +1,67 @@
> > +// SPDX-License-Identifier: GPL-2.0+
> > +/*
> > + * Copyright 2022
> > + * Roger Knecht <rknecht at pm.de>
> > + */
> > +
> > +#include <common.h>
> > +#include <command.h>
> > +#include <fs.h>
> > +#include <malloc.h>
> > +#include <mapmem.h>
> > +
> > +static int do_cat(struct cmd_tbl *cmdtp, int flag, int argc,
> > +               char *const argv[])
> > +{
> > +     char *buffer;
> > +     phys_addr_t buffer_sysmem_addr;

'addr' is shorter

> > +     loff_t file_size;
> > +     int ret;
> > +
> > +     if (argc < 4)
> > +             return CMD_RET_USAGE;
> > +
> > +     // get file size
> > +     ret = fs_set_blk_dev(argv[1], argv[2], FS_TYPE_ANY);
> > +     if (ret)
> > +             return ret;
> > +
> > +     ret = fs_size(argv[3], &file_size);
> > +     if (ret)
> > +             return ret;
> > +
> > +     // allocate memory for file content
> > +     buffer = malloc(file_size + 1);
> > +     if (!buffer)
> > +             return -ENOMEM;
>
> Please, do_cat must only return values from enum command_ret_t.

The easiest way to do this is to put your code (from the 'get file
size' onwards) in a separate function which is called by this
function. It can take args like filename and device. Then when it
returns an error code you can print it and return CMD_RET_FAILURE

>
> > +
> > +     memset(buffer, 0, file_size + 1);
>
> Our calloc() implementation already sets the buffer to zero. So you can
> save one function call.
>
> > +
> > +     // map pointer to system memory
> > +     buffer_sysmem_addr = map_to_sysmem(buffer);
> > +
> > +     // read file to memory
> > +     ret = fs_set_blk_dev(argv[1], argv[2], FS_TYPE_ANY);
> > +     if (ret)
> > +             return ret;
> > +
> > +     ret = fs_read(argv[3], buffer_sysmem_addr, 0, 0, &file_size);
> > +     if (ret)
> > +             return ret;
> > +
> > +     // unmap system memory
> > +     unmap_sysmem(buffer);

Actually map_to_sysmem() does not create a map, so you don't need
this. It is only needed with a call to map_sysmem(). I will send a
patch to update the docs.

> > +
> > +     // print file content
> > +     buffer[file_size] = '\0';
> > +     puts(buffer);
> > +
> > +     free(buffer);
> > +
> > +     return 0;
> > +}
> > +
> > +U_BOOT_CMD(cat, 4, 1, do_cat,
> > +        "print file to standard output",
>
> Please, observe CONFIG_SYS_LONGHELP. Have a look at cmd/bootefi.c or others.

Also how about doc/usage/cmd/cat.rst ?

Regards,
Simon