| Message ID | 20201208041216.888902-5-joel@jms.id.au |
|---|---|
| State | New |
| Headers | show |
| Series | mkimage usability fixes | expand |
Hi Joel Le 08/12/2020 à 05:12, 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 why > their FIT doesn't contain a /signature node, as mkimage will silently > ingore the signing related options. > > Make it obvious that the commands don't work by removing them from the > getopt opt_string. > > $ 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> > -- > v2: Leave padding related options in the CONFIG_FIT_SIGNATURE=y optargs > --- > tools/mkimage.c | 7 +++++-- > 1 file changed, 5 insertions(+), 2 deletions(-) > > diff --git a/tools/mkimage.c b/tools/mkimage.c > index 68d5206cb4fd..f7d3ac40e681 100644 > --- a/tools/mkimage.c > +++ b/tools/mkimage.c > @@ -142,7 +142,11 @@ 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" Option -k and -K is also needed for ciphering. So we should also check FIT_CIPHER. Option -p is "generic", it is used to define the size of the padding. > +#else > +#define OPT_STRING "a:A:b:B:C:d:D:e:Ef:i:ln:O:R:qstT:vVx" > +#endif > static void process_args(int argc, char **argv) > { > char *ptr; > @@ -150,8 +154,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); Regards, Philippe
Hi, On Tue, 8 Dec 2020 at 08:38, Philippe REYNES <philippe.reynes@softathome.com> wrote: > > Hi Joel > > > Le 08/12/2020 à 05:12, 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 why > > their FIT doesn't contain a /signature node, as mkimage will silently > > ingore the signing related options. > > > > Make it obvious that the commands don't work by removing them from the > > getopt opt_string. > > > > $ 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> > > -- > > v2: Leave padding related options in the CONFIG_FIT_SIGNATURE=y optargs > > --- > > tools/mkimage.c | 7 +++++-- > > 1 file changed, 5 insertions(+), 2 deletions(-) > > I have somehow missed these patches on the mailing list. I'm not sure why, but I'm not going to review them as is. Regards, Simon
On Tue, Dec 08, 2020 at 02:42:16PM +1030, Joel Stanley wrote: > 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 why > their FIT doesn't contain a /signature node, as mkimage will silently > ingore the signing related options. > > Make it obvious that the commands don't work by removing them from the > getopt opt_string. > > $ 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> > --- > v2: Leave padding related options in the CONFIG_FIT_SIGNATURE=y optargs > --- > tools/mkimage.c | 7 +++++-- > 1 file changed, 5 insertions(+), 2 deletions(-) > > diff --git a/tools/mkimage.c b/tools/mkimage.c > index 68d5206cb4fd..f7d3ac40e681 100644 > --- a/tools/mkimage.c > +++ b/tools/mkimage.c > @@ -142,7 +142,11 @@ 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:B:C:d:D:e:Ef:i:ln:O:R:qstT:vVx" > +#endif > static void process_args(int argc, char **argv) > { > char *ptr; > @@ -150,8 +154,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); This part of the series breaks a huge number of platforms default build because we don't have 'p' which is: -p => place external data at a static position and not strictly used in signature only options and is used for FIT_EXTERNAL_OFFSET. While I could adjust the patch to keep 'p' in both cases I wanted to bring this up so it's not a surprise and ask you to update just this patch, or suggest things need to be further adjusted. The rest of the series seems fine and is under review. Thanks! -- Tom
diff --git a/tools/mkimage.c b/tools/mkimage.c index 68d5206cb4fd..f7d3ac40e681 100644 --- a/tools/mkimage.c +++ b/tools/mkimage.c @@ -142,7 +142,11 @@ 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:B:C:d:D:e:Ef:i:ln:O:R:qstT:vVx" +#endif static void process_args(int argc, char **argv) { char *ptr; @@ -150,8 +154,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 why their FIT doesn't contain a /signature node, as mkimage will silently ingore the signing related options. Make it obvious that the commands don't work by removing them from the getopt opt_string. $ 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> -- v2: Leave padding related options in the CONFIG_FIT_SIGNATURE=y optargs --- tools/mkimage.c | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) -- 2.29.2