------- Original Message ------- On Friday, August 19th, 2022 at 16:08, Simon Glass <s...@chromium.org> wrote: > > > Hi, Hi Simon,
> > On Thu, 18 Aug 2022 at 11:08, Heinrich Schuchardt xypron.g...@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 rkne...@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 s...@chromium.org > > > S: Maintained > > > F: tools/buildman/ > > > > > > +CAT > > > +M: Roger Knecht rkne...@pm.me > > > +S: Maintained > > > +F: cmd/cat.c > > > + > > > CFI FLASH > > > M: Stefan Roese s...@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 Will be fixed in v5. > > > > + 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 rkne...@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 Ok > > > > + 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. The map_to_sysmem() is used to solve a problem in the sandbox which I described in more detail here: https://lists.denx.de/pipermail/u-boot/2022-June/486883.html Let me know if there is a better solution. > > > > + > > > + // 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 ? I don't understand this remark. There is already a `doc/usage/cmd/cat.rst` in the patch. > > Regards, > Simon Thanks, Roger