From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from phobos.denx.de (phobos.denx.de [85.214.62.61]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id C42EEC77B7F for ; Sat, 13 May 2023 20:17:36 +0000 (UTC) Received: from h2850616.stratoserver.net (localhost [IPv6:::1]) by phobos.denx.de (Postfix) with ESMTP id 1970284785; Sat, 13 May 2023 22:17:34 +0200 (CEST) Authentication-Results: phobos.denx.de; dmarc=pass (p=none dis=none) header.from=amarulasolutions.com Authentication-Results: phobos.denx.de; spf=pass smtp.mailfrom=u-boot-bounces@lists.denx.de Authentication-Results: phobos.denx.de; dkim=pass (1024-bit key; unprotected) header.d=amarulasolutions.com header.i=@amarulasolutions.com header.b="PiRvp0m+"; dkim-atps=neutral Received: by phobos.denx.de (Postfix, from userid 109) id 4BA8084706; Sat, 13 May 2023 22:17:33 +0200 (CEST) Received: from mail-wm1-x332.google.com (mail-wm1-x332.google.com [IPv6:2a00:1450:4864:20::332]) (using TLSv1.3 with cipher TLS_AES_128_GCM_SHA256 (128/128 bits)) (No client certificate requested) by phobos.denx.de (Postfix) with ESMTPS id 10EA885494 for ; Sat, 13 May 2023 22:17:28 +0200 (CEST) Authentication-Results: phobos.denx.de; dmarc=pass (p=none dis=none) header.from=amarulasolutions.com Authentication-Results: phobos.denx.de; spf=pass smtp.mailfrom=francis.laniel@amarulasolutions.com Received: by mail-wm1-x332.google.com with SMTP id 5b1f17b1804b1-3f41d087b3bso83166655e9.0 for ; Sat, 13 May 2023 13:17:28 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=amarulasolutions.com; s=google; t=1684009047; x=1686601047; h=content-transfer-encoding:mime-version:references:in-reply-to :message-id:date:subject:cc:to:from:from:to:cc:subject:date :message-id:reply-to; bh=6vbyXjhYso10/ygA9IVaysg4HYRKnyVYT58yz4Pe3iw=; b=PiRvp0m+CPNYXFA+Fa6FV+Fz5y0U+J/uh2Rd1q1nWUgsRB6RNMcDA3CeSsmL4afYfO nDzPMHi+yPWHk1oXAMJ4OWFszGkz3ubwV9qvI0lrrnjr03p0fYbjsbfgZ0JvLj8y3G06 3CopcxHsDPnhqoQtvnZeMDD3+em7rpKxwUNVE= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20221208; t=1684009047; x=1686601047; h=content-transfer-encoding:mime-version:references:in-reply-to :message-id:date:subject:cc:to:from:x-gm-message-state:from:to:cc :subject:date:message-id:reply-to; bh=6vbyXjhYso10/ygA9IVaysg4HYRKnyVYT58yz4Pe3iw=; b=lqILngjr3T9tg81KolyFkmNvrbzqCSUoub+07TBCvl9eCAJy+jlVGRxeANH+mPxQGS 70Ng668MRaVDEZ6oiDO6o5jHA1kENGrGqY4TcdX0IkGiThZ6bo0+KhbVGA5AsgA/OKHe VwJAFRfjw5yilmvdmk503FzwSuGDzbcjlV/QF+Y+HUL/TNNvED2HnD78IoTmnuncoudx 8DFmHFAdM9D6wbesOlhFonxSCDHvKM9yAA8v9HKBJSCQVwiRZl2ylLGc8/DC5TuITpQ8 hhr7GmCLhGDH5gdWGuvtuB/HEqpOEHg2i2W66eS5LsBOuML8fuf/dFWYEb4g+3swiCRA V2zA== X-Gm-Message-State: AC+VfDwAbO2EbQpENV2CpTJFBtkfiALmqkQwqRd8iOGhWxntiU6Adm95 KC4Ncsjtwxlks+27vRfuGkSexA== X-Google-Smtp-Source: ACHHUZ5/p3cPP1mWZy72fH55qSPN3ycqQbdUD5Tx5gYuIPlqP01vzB2inbcCnqlyI9HIESGXqBjbIw== X-Received: by 2002:a5d:5266:0:b0:2e9:bb2f:ce03 with SMTP id l6-20020a5d5266000000b002e9bb2fce03mr22264325wrc.0.1684009047349; Sat, 13 May 2023 13:17:27 -0700 (PDT) Received: from pwmachine.localnet ([213.94.16.214]) by smtp.gmail.com with ESMTPSA id m2-20020a056000008200b0030630120e56sm27092438wrx.57.2023.05.13.13.17.25 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Sat, 13 May 2023 13:17:26 -0700 (PDT) From: Francis Laniel To: Heinrich Schuchardt Cc: Michael Nazzareno Trimarchi , Tom Rini , Simon Glass , Harald Seiler , Ilias Apalodimas , Neil Armstrong , Roger Knecht , Masahisa Kojima , Alexey Romanov , Dzmitry Sankouski , Evgeny Bachinin , Marek Vasut , Hector Palacios , Sughosh Ganu , Rasmus Villemoes , u-boot@lists.denx.de Subject: Re: [RFC PATCH v8 11/23] cmd: Add new cli command Date: Sat, 13 May 2023 21:17:24 +0100 Message-ID: <5933749.lOV4Wx5bFT@pwmachine> In-Reply-To: <9f6c140d-fe19-aaa7-e7cb-fef2a8c09f02@gmx.de> References: <20230512200331.51457-1-francis.laniel@amarulasolutions.com> <20230512200331.51457-12-francis.laniel@amarulasolutions.com> <9f6c140d-fe19-aaa7-e7cb-fef2a8c09f02@gmx.de> MIME-Version: 1.0 Content-Transfer-Encoding: quoted-printable Content-Type: text/plain; charset="iso-8859-1" X-BeenThere: u-boot@lists.denx.de X-Mailman-Version: 2.1.39 Precedence: list List-Id: U-Boot discussion List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: u-boot-bounces@lists.denx.de Sender: "U-Boot" X-Virus-Scanned: clamav-milter 0.103.8 at phobos.denx.de X-Virus-Status: Clean Hi. Le samedi 13 mai 2023, 02:18:27 WEST Heinrich Schuchardt a =E9crit : > On 5/12/23 22:03, Francis Laniel wrote: > > This command can be used to print the current parser with 'cli print'. > > It can also be used to set the current parser with 'cli set'. > > For the moment, only one value is valid for set: old. >=20 > What would be the benefit having multiple alternative parsers in one > U-Boot instance? I do not think we should push this contribution as the default parser for t= he=20 moment. I mean, once this contribution would be merged, we should stick with the tw= o=20 parsers (the new and old) for a bit of time before deprecating the old one = in=20 favor of the new one. So, permit switching of parser at runtime if the perfect help for developer= s=20 to check their boards behave correctly with the new parser. > I would prefer the solution to stay simple and small meaning just one > parser per U-Boot binary. You can perfectly have only one parser per U-Boot binary. To do so, you just need to check only one parser, you will then not be able= to=20 change the parser at runtime: =3D> cli get 2021 =3D> cli set old Want to set current parser to old, but its code was not compiled! So, when developing for your board, just enable the two parser to check whi= ch=20 one suits your needs. Then, when you release, just release only with the CONFIG_HUSH_*_PARSER whi= ch=20 suits your needs. > > Signed-off-by: Francis Laniel > > --- > >=20 > > cmd/Makefile | 2 + > > cmd/cli.c | 120 ++++++++++++++++++++++++++++++++++++++++++ > > common/cli.c | 3 +- > > doc/usage/cmd/cli.rst | 59 +++++++++++++++++++++ > > doc/usage/index.rst | 1 + > > 5 files changed, 184 insertions(+), 1 deletion(-) > > create mode 100644 cmd/cli.c > > create mode 100644 doc/usage/cmd/cli.rst > >=20 > > diff --git a/cmd/Makefile b/cmd/Makefile > > index 6c37521b4e..706e934836 100644 > > --- a/cmd/Makefile > > +++ b/cmd/Makefile > > @@ -224,6 +224,8 @@ obj-$(CONFIG_CMD_AVB) +=3D avb.o > >=20 > > # Foundries.IO SCP03 > > obj-$(CONFIG_CMD_SCP03) +=3D scp03.o > >=20 > > +obj-$(CONFIG_HUSH_PARSER) +=3D cli.o > > + > >=20 > > obj-$(CONFIG_ARM) +=3D arm/ > > obj-$(CONFIG_RISCV) +=3D riscv/ > > obj-$(CONFIG_SANDBOX) +=3D sandbox/ > >=20 > > diff --git a/cmd/cli.c b/cmd/cli.c > > new file mode 100644 > > index 0000000000..7671785b83 > > --- /dev/null > > +++ b/cmd/cli.c > > @@ -0,0 +1,120 @@ > > +// SPDX-License-Identifier: GPL-2.0+ > > + > > +#include > > +#include > > +#include > > +#include > > +#include > > + > > +DECLARE_GLOBAL_DATA_PTR; > > + > > +static const char *gd_flags_to_parser(void) > > +{ > > + if (gd->flags & GD_FLG_HUSH_OLD_PARSER) > > + return "old"; > > + return NULL; > > +} > > + > > +static int do_cli_get(struct cmd_tbl *cmdtp, int flag, int argc, > > + char *const argv[]) > > +{ > > + const char *current =3D gd_flags_to_parser(); > > + > > + if (!current) { > > + printf("current cli value is not valid, this should not happen! \n"); >=20 > Please, use log_err() for errors. We want to keep our code small. "this > should not happen!" does not convey any additional information. >=20 > > + return CMD_RET_FAILURE; > > + } > > + > > + printf("%s\n", current); > > + > > + return CMD_RET_SUCCESS; > > +} > > + > > +static int parser_string_to_gd_flags(const char *parser) > > +{ > > + if (!strcmp(parser, "old")) > > + return GD_FLG_HUSH_OLD_PARSER; > > + return -1; > > +} > > + > > +static void reset_parser_gd_flags(void) > > +{ > > + gd->flags &=3D ~GD_FLG_HUSH_OLD_PARSER; > > +} > > + > > +static int do_cli_set(struct cmd_tbl *cmdtp, int flag, int argc, > > + char *const argv[]) > > +{ > > + char *parser_name; > > + int parser_flag; > > + > > + if (argc < 2) > > + return CMD_RET_USAGE; > > + > > + parser_name =3D argv[1]; > > + > > + parser_flag =3D parser_string_to_gd_flags(parser_name); > > + if (parser_flag =3D=3D -1) { > > + printf("Bad value for parser name: %s\n", parser_name); > > + return CMD_RET_USAGE; > > + } > > + > > + if (parser_flag =3D=3D GD_FLG_HUSH_OLD_PARSER && > > + !CONFIG_IS_ENABLED(HUSH_OLD_PARSER)) { > > + printf("Want to set current parser to old, but its code was not > > compiled!\n"); + return CMD_RET_FAILURE; > > + } >=20 > We want to keep the code small. We don't need different strings "Parser > %d is not available" would be enough information. >=20 > > +. > > + if (parser_flag =3D=3D GD_FLG_HUSH_2021_PARSER && > > + !CONFIG_IS_ENABLED(HUSH_2021_PARSER)) { > > + printf("Want to set current parser to 2021, but its code was=20 not > > compiled!\n"); + return CMD_RET_FAILURE; > > + } > > + > > + reset_parser_gd_flags(); > > + gd->flags |=3D parser_flag; > > + > > + cli_init(); > > + cli_loop(); > > + > > + /* cli_loop() should never return. */ > > + return CMD_RET_FAILURE; > > +} > > + > > +static struct cmd_tbl parser_sub[] =3D { > > + U_BOOT_CMD_MKENT(get, 1, 1, do_cli_get, "", ""), > > + U_BOOT_CMD_MKENT(set, 2, 1, do_cli_set, "", ""), > > +}; > > + > > +static int do_cli(struct cmd_tbl *cmdtp, int flag, int argc, > > + char *const argv[]) > > +{ > > + struct cmd_tbl *cp; > > + > > + if (argc < 2) > > + return CMD_RET_USAGE; > > + > > + /* drop initial "parser" arg */ > > + argc--; > > + argv++; > > + > > + cp =3D find_cmd_tbl(argv[0], parser_sub, ARRAY_SIZE(parser_sub)); > > + if (cp) > > + return cp->cmd(cmdtp, flag, argc, argv); > > + > > + return CMD_RET_USAGE; > > +} > > + > > +#if CONFIG_IS_ENABLED(SYS_LONGHELP) > > +static char cli_help_text[] =3D > > + "get - print current cli\n" > > + "set - set the current cli, possible value is: old" > > + ; > > +#endif > > + > > +U_BOOT_CMD(cli, 3, 1, do_cli, > > + "cli", > > +#if CONFIG_IS_ENABLED(SYS_LONGHELP) > > + cli_help_text > > +#endif > > +); > > diff --git a/common/cli.c b/common/cli.c > > index e5fe1060d0..d419671e8c 100644 > > --- a/common/cli.c > > +++ b/common/cli.c > > @@ -268,7 +268,8 @@ void cli_loop(void) > >=20 > > void cli_init(void) > > { > > #ifdef CONFIG_HUSH_PARSER > >=20 > > - if (!(gd->flags & GD_FLG_HUSH_OLD_PARSER)) > > + if (!(gd->flags & GD_FLG_HUSH_OLD_PARSER) >=20 > This constraint is superfluous. >=20 > Best regards >=20 > Heinrich >=20 > > + && CONFIG_IS_ENABLED(HUSH_OLD_PARSER)) > >=20 > > gd->flags |=3D GD_FLG_HUSH_OLD_PARSER; > > =09 > > u_boot_hush_start(); > > =20 > > #endif > >=20 > > diff --git a/doc/usage/cmd/cli.rst b/doc/usage/cmd/cli.rst > > new file mode 100644 > > index 0000000000..89ece3203d > > --- /dev/null > > +++ b/doc/usage/cmd/cli.rst > > @@ -0,0 +1,59 @@ > > +.. SPDX-License-Identifier: GPL-2.0+ > > + > > +cli command > > +=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D > > + > > +Synopis > > +------- > > + > > +:: > > + > > + cli get > > + cli set cli_flavor > > + > > +Description > > +----------- > > + > > +The cli command permits getting and changing the current parser at > > runtime. + > > +cli get > > +~~~~~~~ > > + > > +It shows the current value of the parser used by the CLI. > > + > > +cli set > > +~~~~~~~ > > + > > +It permits setting the value of the parser used by the CLI. > > + > > +Possible values are old and 2021. > > +Note that, to use a specific parser its code should have been compiled, > > that +is to say you need to enable the corresponding CONFIG_HUSH*. > > +Otherwise, an error message is printed. > > + > > +Examples > > +-------- > > + > > +Get the current parser:: > > + > > + =3D> cli get > > + old > > + > > +Change the current parser:: > > + > > + =3D> cli set old > > + > > +Trying to set the current parser to an unknown value:: > > + > > + =3D> cli set foo > > + Bad value for parser name: foo > > + cli - cli > > + > > + Usage: > > + cli get - print current cli > > + set - set the current cli, possible value is: old > > + > > +Return value > > +------------ > > + > > +The return value $? indicates whether the command succeeded. > > diff --git a/doc/usage/index.rst b/doc/usage/index.rst > > index 0fde130a54..d0e26d1aee 100644 > > --- a/doc/usage/index.rst > > +++ b/doc/usage/index.rst > > @@ -37,6 +37,7 @@ Shell commands > >=20 > > cmd/bootz > > cmd/cat > > cmd/cbsysinfo > >=20 > > + cmd/cli > >=20 > > cmd/cls > > cmd/cmp > > cmd/coninfo