| Message ID | 20201111111833.741937-4-joel@jms.id.au |
|---|---|
| State | Superseded |
| Delegated to: | Tom Rini |
| Headers | show |
| Series | mkimage usability fixes | expand |
Hi Joel, Sorry for this very late answer. You're right, this is a bad behaviour, but I think that this solution remove too many options. For example, If signature is disabled, this solution also disable the padding in the fit image. Looking a bit deeper, this patch removes all options that are not displayed by the help of mkimage when signature is disabled. But I think that this help is not correct. If the signature is disabled, the padding is still available. So I think that we have an issue in the help too. Regards, Philippe Le 11/11/2020 à 12:18, Joel Stanley a écrit : > When CONFIG_FIT_SIGNATURE=n the signing options are not available. If a > user is careful they will notice this when looking at the help output. > > If they are not careful they will waste several hours wondering what > they got wrong, as mkimage will silently ignore the signing related > options. > > Make it obvious that the commands don't work by removing them from the > getopt opt_string. > > The tool will now behave as follows: > > $ mkimage -f machine.its -k keys -K u-boot-pubkey.dtb -r image.fit > mkimage: invalid option -- 'k' > Error: Invalid option > > Signed-off-by: Joel Stanley <joel@jms.id.au> > --- > tools/mkimage.c | 9 +++++++-- > 1 file changed, 7 insertions(+), 2 deletions(-) > > diff --git a/tools/mkimage.c b/tools/mkimage.c > index e78608293e72..10a1b3dc8c18 100644 > --- a/tools/mkimage.c > +++ b/tools/mkimage.c > @@ -142,6 +142,12 @@ static int add_content(int type, const char *fname) > return 0; > } > > +#ifdef CONFIG_FIT_SIGNATURE > +#define OPT_STRING "a:A:b:B:c:C:d:D:e:Ef:Fk:i:K:ln:N:p:O:rR:qstT:vVx" > +#else > +#define OPT_STRING "a:A:b:C:d:D:e:f:i:ln:O:R:qstT:vVx" > +#endif > + > static void process_args(int argc, char **argv) > { > char *ptr; > @@ -149,8 +155,7 @@ static void process_args(int argc, char **argv) > char *datafile = NULL; > int opt; > > - while ((opt = getopt(argc, argv, > - "a:A:b:B:c:C:d:D:e:Ef:Fk:i:K:ln:N:p:O:rR:qstT:vVx")) != -1) { > + while ((opt = getopt(argc, argv, OPT_STRING)) != -1) { > switch (opt) { > case 'a': > params.addr = strtoull(optarg, &ptr, 16);
On Mon, 7 Dec 2020 at 17:57, Philippe REYNES <philippe.reynes@softathome.com> wrote: > > Hi Joel, > > Sorry for this very late answer. > > You're right, this is a bad behaviour, but I think that this > solution remove too many options. For example, If signature > is disabled, this solution also disable the padding in the fit image. Ok. I can amend my patch but it wasn't obvious which commands should be moved from outside the FIT_SIGNATURE guard. Just -E and -B? > > Looking a bit deeper, this patch removes all options that are > not displayed by the help of mkimage when signature is disabled. > But I think that this help is not correct. If the signature is disabled, > the padding is still available. > So I think that we have an issue in the help too. > > Regards, > Philippe > > Le 11/11/2020 à 12:18, Joel Stanley a écrit : > > When CONFIG_FIT_SIGNATURE=n the signing options are not available. If a > > user is careful they will notice this when looking at the help output. > > > > If they are not careful they will waste several hours wondering what > > they got wrong, as mkimage will silently ignore the signing related > > options. > > > > Make it obvious that the commands don't work by removing them from the > > getopt opt_string. > > > > The tool will now behave as follows: > > > > $ mkimage -f machine.its -k keys -K u-boot-pubkey.dtb -r image.fit > > mkimage: invalid option -- 'k' > > Error: Invalid option > > > > Signed-off-by: Joel Stanley <joel@jms.id.au> > > --- > > tools/mkimage.c | 9 +++++++-- > > 1 file changed, 7 insertions(+), 2 deletions(-) > > > > diff --git a/tools/mkimage.c b/tools/mkimage.c > > index e78608293e72..10a1b3dc8c18 100644 > > --- a/tools/mkimage.c > > +++ b/tools/mkimage.c > > @@ -142,6 +142,12 @@ static int add_content(int type, const char *fname) > > return 0; > > } > > > > +#ifdef CONFIG_FIT_SIGNATURE > > +#define OPT_STRING "a:A:b:B:c:C:d:D:e:Ef:Fk:i:K:ln:N:p:O:rR:qstT:vVx" > > +#else > > +#define OPT_STRING "a:A:b:C:d:D:e:f:i:ln:O:R:qstT:vVx" > > +#endif > > + > > static void process_args(int argc, char **argv) > > { > > char *ptr; > > @@ -149,8 +155,7 @@ static void process_args(int argc, char **argv) > > char *datafile = NULL; > > int opt; > > > > - while ((opt = getopt(argc, argv, > > - "a:A:b:B:c:C:d:D:e:Ef:Fk:i:K:ln:N:p:O:rR:qstT:vVx")) != -1) { > > + while ((opt = getopt(argc, argv, OPT_STRING)) != -1) { > > switch (opt) { > > case 'a': > > params.addr = strtoull(optarg, &ptr, 16);
diff --git a/tools/mkimage.c b/tools/mkimage.c index e78608293e72..10a1b3dc8c18 100644 --- a/tools/mkimage.c +++ b/tools/mkimage.c @@ -142,6 +142,12 @@ static int add_content(int type, const char *fname) return 0; } +#ifdef CONFIG_FIT_SIGNATURE +#define OPT_STRING "a:A:b:B:c:C:d:D:e:Ef:Fk:i:K:ln:N:p:O:rR:qstT:vVx" +#else +#define OPT_STRING "a:A:b:C:d:D:e:f:i:ln:O:R:qstT:vVx" +#endif + static void process_args(int argc, char **argv) { char *ptr; @@ -149,8 +155,7 @@ static void process_args(int argc, char **argv) char *datafile = NULL; int opt; - while ((opt = getopt(argc, argv, - "a:A:b:B:c:C:d:D:e:Ef:Fk:i:K:ln:N:p:O:rR:qstT:vVx")) != -1) { + while ((opt = getopt(argc, argv, OPT_STRING)) != -1) { switch (opt) { case 'a': params.addr = strtoull(optarg, &ptr, 16);
When CONFIG_FIT_SIGNATURE=n the signing options are not available. If a user is careful they will notice this when looking at the help output. If they are not careful they will waste several hours wondering what they got wrong, as mkimage will silently ignore the signing related options. Make it obvious that the commands don't work by removing them from the getopt opt_string. The tool will now behave as follows: $ mkimage -f machine.its -k keys -K u-boot-pubkey.dtb -r image.fit mkimage: invalid option -- 'k' Error: Invalid option Signed-off-by: Joel Stanley <joel@jms.id.au> --- tools/mkimage.c | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-)