Re: [PATCH v3] cmd: cat: add new command

Hi,

On Thu, 18 Aug 2022 at 11:08, Heinrich Schuchardt xypron.glpk@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@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@chromium.org S: Maintained F: tools/buildman/

+CAT +M: Roger Knecht rknecht@pm.me +S: Maintained +F: cmd/cat.c

• CFI FLASH M: Stefan Roese sr@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@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