[PATCH v2] cmd: cat: add new command

Sun Jun 19 16:23:19 CEST 2022

On Thursday, June 9th, 2022 at 22:27, Roger Knecht <rknecht at pm.me> wrote:
> On Wednesday, June 8th, 2022 at 15:59, Tom Rini trini at konsulko.com wrote:
>
> > On Sat, Jun 04, 2022 at 11:19:15AM +0000, Roger Knecht wrote:
> >
> > > Add cat command to print file content to standard out
> > >
> > > Signed-off-by: Roger Knecht rknecht at pm.me
> > > ---
> > > 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 | 58 ++++++++++++++++++++++++++++++++++++++++++++++++++++
> > > 4 files changed, 70 insertions(+)
> > > create mode 100644 cmd/cat.c
> > >
> > > diff --git a/MAINTAINERS b/MAINTAINERS
> > > index 56be0bfad0..7c5cd178d9 100644
> > > --- a/MAINTAINERS
> > > +++ b/MAINTAINERS
> > > @@ -729,6 +729,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 69c1814d24..8b531c7ebe 100644
> > > --- a/cmd/Kconfig
> > > +++ b/cmd/Kconfig
> > > @@ -1492,6 +1492,12 @@ endmenu
> > >
> > > menu "Shell scripting commands"
> > >
> > > +config CMD_CAT
> > > + bool "cat"
> > > + default y
> >
> > New commands shouldn't be default enabled. I also don't see a test.
> > Please add a test, and enable the command in sandbox so the test is run.
> > Thanks!
>
>
> Thanks for the review. Will be fixed in the next version.

Hi Tom,

I'm running into issues when writing a test for cat:

1. malloc() allocates memory outside the sandbox emulated DRAM which causes fs_read() to fail:

relevant code: (paddr - allocated by malloc() - is not within the gd->ram_size)
```
void *phys_to_virt(phys_addr_t paddr)
{
[...]

    /* If the address is within emulated DRAM, calculate the value */
    if (paddr < gd->ram_size)
        return (void *)(gd->arch.ram_buf + paddr);

[...]

    printf("%s: Cannot map sandbox address %lx (SDRAM from 0 to %lx)\n",
           __func__, (ulong)paddr, (ulong)gd->ram_size);
    os_abort();

[...]

```
output:
phys_to_virt: Cannot map sandbox address 140093b0 (SDRAM from 0 to 8000000)

trace:
#0  __GI_raise (sig=sig at entry=6) at ../sysdeps/unix/sysv/linux/raise.c:51
#1  0x00007ffff74e87f1 in __GI_abort () at abort.c:79
#2  0x000055555558b352 in os_abort () at arch/sandbox/cpu/os.c:948
#3  0x000055555558c093 in phys_to_virt (paddr=paddr at entry=335582128) at arch/sandbox/cpu/cpu.c:118
#4  0x000055555558c1c2 in map_physmem (paddr=paddr at entry=335582128, len=<optimized out>, len at entry=0, flags=flags at entry=0) at arch/sandbox/cpu/cpu.c:179
#5  0x000055555564a5b6 in map_sysmem (len=0, paddr=335582128) at ./arch/sandbox/include/asm/io.h:36
#6  _fs_read (filename=0x1400c5d0 "file", addr=addr at entry=335582128, offset=offset at entry=0, len=len at entry=0, do_lmb_check=do_lmb_check at entry=0, actread=actread at entry=0x7fffffffe740) at fs/fs.c:596
#7  0x000055555564a68c in fs_read (filename=<optimized out>, addr=addr at entry=335582128, offset=offset at entry=0, len=len at entry=0, actread=actread at entry=0x7fffffffe740) at fs/fs.c:611
#8  0x00005555555a2a77 in do_cat (cmdtp=<optimized out>, flag=<optimized out>, argc=<optimized out>, argv=0x1400c650) at cmd/cat.c:42
#9  0x00005555555de75e in cmd_always_repeatable (cmdtp=<optimized out>, flag=<optimized out>, argc=<optimized out>, argv=<optimized out>, repeatable=<optimized out>) at common/command.c:544
#10 0x00005555555ddb56 in cmd_call (cmdtp=cmdtp at entry=0x555555accbe8 <_u_boot_list_2_cmd_2_cat>, flag=flag at entry=0, argc=argc at entry=4, argv=argv at entry=0x1400c650, repeatable=repeatable at entry=0x7fffffffe794) at common/command.c:580
#11 0x00005555555de87c in cmd_process (flag=<optimized out>, flag at entry=0, argc=4, argv=0x1400c650, repeatable=repeatable at entry=0x555555af64d4 <flag_repeat>, ticks=ticks at entry=0x0) at common/command.c:635
#12 0x00005555555cb9fd in run_pipe_real (pi=pi at entry=0x1400c500) at common/cli_hush.c:1676
#13 0x00005555555cbdc5 in run_list_real (pi=pi at entry=0x1400c500) at common/cli_hush.c:1873
#14 0x00005555555cbedc in run_list (pi=0x1400c500) at common/cli_hush.c:2022
#15 0x00005555555cc100 in parse_stream_outer (inp=inp at entry=0x7fffffffe950, flag=flag at entry=2) at common/cli_hush.c:3206
#16 0x00005555555cc183 in parse_file_outer () at common/cli_hush.c:3289
#17 0x00005555555dd90f in cli_loop () at common/cli.c:229
#18 0x00005555555c9acc in main_loop () at common/main.c:69
#19 0x00005555555ccfd4 in run_main_loop () at common/board_r.c:590
#20 0x00005555555cd32d in initcall_run_list (init_sequence=0x555555ae9220 <init_sequence_r>) at include/initcall.h:46
#21 board_init_r (new_gd=<optimized out>, dest_addr=<optimized out>) at common/board_r.c:824
#22 0x0000555555589e34 in main (argc=<optimized out>, argv=0x7fffffffed58) at arch/sandbox/cpu/start.c:529

Is this a bug in the malloc implementation or does the fs_read() buffer require a different allocation?

2. Testing the cat command requires a filesystem. How should the filesystem be provided for tests?

Currently I test the command in the sandbox by:
$ ./u-boot
=> host bind 0 disk.ext2
=> cat host 0 file

Shipping a binary filesystem in the patch is not a great idea.
Alternatively the filesystem could be created as part of the test.
What is your recommendation?

Thanks,
Roger