[PATCH v3] cmd: cat: add new command

Sun Aug 21 16:34:55 CEST 2022

------- Original Message -------
On Sunday, August 21st, 2022 at 14:18, Simon Glass <sjg at chromium.org> wrote:
> 
> 
> Hi Roger,
> 
> On Sun, 21 Aug 2022 at 07:27, Roger Knecht rknecht at pm.me wrote:
> 
> > ------- Original Message -------
> > On Friday, August 19th, 2022 at 16:08, Simon Glass sjg at chromium.org wrote:
> > 
> > > Hi,
> > > Hi Simon,
> > 
> > > 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:map_to_sysmem
> > > > > - 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
> > > 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 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
> > > 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.
> 
> 
> Actually I was talking the unmap_sysmem(), the line above my comment.
> The unmap is used after a map, where as the map_to_sysmem() is doing
> something different. Yes you need map_to_sysmem(), but you should not
> need the unmap_sysmem(). If I am missing something, let me know.
> 
> [..]
> 
> > > 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.
> 
> 
> Yes sorry about that. I thought I deleted that line.
> 
> Regards,
> Simon

Thanks for the clarification Simon.
I will remove the unmap_sysmem() call in the next patch.

Regards,
Roger