Patchwork [U-Boot,v3] mkimage: add "-V" option to print version information

login
register
mail settings
Submitter Wolfgang Denk
Date Feb. 11, 2011, 10:41 p.m.
Message ID <1297464103-9111-1-git-send-email-wd@denx.de>
Download mbox | patch
Permalink /patch/82843/
State Superseded
Headers show

Comments

Wolfgang Denk - Feb. 11, 2011, 10:41 p.m.
Signed-off-by: Wolfgang Denk <wd@denx.de>
---
v2: fix missing argument to printf() call.
v3: explain the magic "+ 7" offset into the version string

 tools/mkimage.c |   11 +++++++++++
 1 files changed, 11 insertions(+), 0 deletions(-)
Kim Phillips - Feb. 11, 2011, 11:11 p.m.
On Fri, 11 Feb 2011 23:41:43 +0100
Wolfgang Denk <wd@denx.de> wrote:

> +			case 'V':
> +				/*
> +				 * Skip the "U-Boot " part in
> +				 * U_BOOT_VERSION by adding 7
> +				 */
> +				printf("mkimage version %s\n",
> +					U_BOOT_VERSION + 7);

I'd have done it without magic nor comments as

U_BOOT_VERSION[sizeof("U-Boot ") + 1]

or even

U_BOOT_VERSION[strlen("U-Boot ")]

Kim
Albert ARIBAUD - Feb. 12, 2011, 6:47 a.m.
Le 12/02/2011 00:11, Kim Phillips a écrit :
> On Fri, 11 Feb 2011 23:41:43 +0100
> Wolfgang Denk<wd@denx.de>  wrote:
>
>> +			case 'V':
>> +				/*
>> +				 * Skip the "U-Boot " part in
>> +				 * U_BOOT_VERSION by adding 7
>> +				 */
>> +				printf("mkimage version %s\n",
>> +					U_BOOT_VERSION + 7);

Now I get it. :)

I might argue that this is kind of a hack, and that rather than trying 
to prune the U_BOOT_VERSION string, one should define two macros, for 
instance:

	#define PLAIN_VERSION "whatever"
	#define U_BOOT_VERSION "U-Boot " PLAIN_VERSION

... which has the advantage of not affecting the existing code, or a 
cleaner, but more invasive, change...

	#define U_BOOT_VERSION "whatever" /* without "U-Boot " */
	#define U_BOOT_VERSION_BANNER "U-Boot " U_BOOT_VERSION

... and use the right macro for the right need.

> I'd have done it without magic nor comments as
>
> U_BOOT_VERSION[sizeof("U-Boot ") + 1]
>
> or even
>
> U_BOOT_VERSION[strlen("U-Boot ")]

The second one calls strlen() at run-time, plus it allocates the "U-Boot 
" string for no justifiable reason -- assuming the first one can be 
compile-time evaluated by the compiler, of course.

> Kim

Amicalement,
Wolfgang Denk - Feb. 12, 2011, 9:29 a.m.
Dear Albert ARIBAUD,

In message <4D562D0A.9060800@free.fr> you wrote:
>
> I might argue that this is kind of a hack, and that rather than trying 
> to prune the U_BOOT_VERSION string, one should define two macros, for 
> instance:
> 
> 	#define PLAIN_VERSION "whatever"
> 	#define U_BOOT_VERSION "U-Boot " PLAIN_VERSION

I know this works find in C, but I'm not absolutely sure if this is
acceptable with all assemblers.

But I get the idea...

Updated patch follows.

Best regards,

Wolfgang Denk
Wolfgang Denk - Feb. 12, 2011, 9:37 a.m.
Dear Kim Phillips,

In message <20110211171117.b9b05b01.kim.phillips@freescale.com> you wrote:
> On Fri, 11 Feb 2011 23:41:43 +0100
> Wolfgang Denk <wd@denx.de> wrote:
> 
> > +			case 'V':
> > +				/*
> > +				 * Skip the "U-Boot " part in
> > +				 * U_BOOT_VERSION by adding 7
> > +				 */
> > +				printf("mkimage version %s\n",
> > +					U_BOOT_VERSION + 7);
> 
> I'd have done it without magic nor comments as
> 
> U_BOOT_VERSION[sizeof("U-Boot ") + 1]
> 
> or even
> 
> U_BOOT_VERSION[strlen("U-Boot ")]

Hm... I would have cleaned up such locations in U-Boot as part of my
v4 patch that introduces PLAIN_VERSION - if I had found any such
code. Am I missing something?

Best regards,

Wolfgang Denk
Kim Phillips - Feb. 12, 2011, 10:04 p.m.
On Sat, 12 Feb 2011 07:47:38 +0100
Albert ARIBAUD <albert.aribaud@free.fr> wrote:

> Le 12/02/2011 00:11, Kim Phillips a écrit :
> > I'd have done it without magic nor comments as
> >
> > U_BOOT_VERSION[sizeof("U-Boot ") + 1]
> >
> > or even
> >
> > U_BOOT_VERSION[strlen("U-Boot ")]
> 
> The second one calls strlen() at run-time, plus it allocates the "U-Boot 
> " string for no justifiable reason -- assuming the first one can be 
> compile-time evaluated by the compiler, of course.

no, the compiler evaluates the strlen at compile time and eliminates
the need to allocate the "U-Boot " string in both cases.

that's not to say that there aren't a couple of gaffes above: the
sizeof needs a - 1 instead of a + 1 (because it includes the trailing
'\0'), and both expressions need to be prepended with an '&'.  So, we
have something like:

&U_BOOT_VERSION[sizeof("U-Boot ") - 1]

or

&U_BOOT_VERSION[strlen("U-Boot ")]

Kim
Kim Phillips - Feb. 12, 2011, 10:17 p.m.
On Sat, 12 Feb 2011 10:37:16 +0100
Wolfgang Denk <wd@denx.de> wrote:

> Dear Kim Phillips,
> 
> In message <20110211171117.b9b05b01.kim.phillips@freescale.com> you wrote:
> > On Fri, 11 Feb 2011 23:41:43 +0100
> > Wolfgang Denk <wd@denx.de> wrote:
> > 
> > > +			case 'V':
> > > +				/*
> > > +				 * Skip the "U-Boot " part in
> > > +				 * U_BOOT_VERSION by adding 7
> > > +				 */
> > > +				printf("mkimage version %s\n",
> > > +					U_BOOT_VERSION + 7);
> > 
> > I'd have done it without magic nor comments as
> > 
> > U_BOOT_VERSION[sizeof("U-Boot ") + 1]
> > 
> > or even
> > 
> > U_BOOT_VERSION[strlen("U-Boot ")]
> 
> Hm... I would have cleaned up such locations in U-Boot as part of my
> v4 patch that introduces PLAIN_VERSION - if I had found any such
> code. Am I missing something?

ok, apart from the missing & and +/- gaffes, what are you talking
about?  The code does exactly what the + 7 does, except it's more
self-explanatory.

Kim
Wolfgang Denk - Feb. 12, 2011, 11:31 p.m.
Dear Kim Phillips,

In message <20110212161715.111c368f.kim.phillips@freescale.com> you wrote:
>
> > > I'd have done it without magic nor comments as
...
> > Hm... I would have cleaned up such locations in U-Boot as part of my
> > v4 patch that introduces PLAIN_VERSION - if I had found any such
> > code. Am I missing something?
> 
> ok, apart from the missing & and +/- gaffes, what are you talking
> about?  The code does exactly what the + 7 does, except it's more
> self-explanatory.

Sorry, my fault. I've misread your "I'd have done it" as "I have done
it" but did not find any such code.  Please ignore me.

Best regards,

Wolfgang Denk

Patch

diff --git a/tools/mkimage.c b/tools/mkimage.c
index f5859d7..febb536 100644
--- a/tools/mkimage.c
+++ b/tools/mkimage.c
@@ -23,6 +23,7 @@ 
 
 #include "mkimage.h"
 #include <image.h>
+#include <version.h>
 
 static void copy_file(int, const char *, int);
 static void usage(void);
@@ -246,6 +247,14 @@  main (int argc, char **argv)
 			case 'v':
 				params.vflag++;
 				break;
+			case 'V':
+				/*
+				 * Skip the "U-Boot " part in
+				 * U_BOOT_VERSION by adding 7
+				 */
+				printf("mkimage version %s\n",
+					U_BOOT_VERSION + 7);
+				exit(EXIT_SUCCESS);
 			case 'x':
 				params.xflag++;
 				break;
@@ -590,6 +599,8 @@  usage ()
 		params.cmdname);
 	fprintf (stderr, "       %s [-D dtc_options] -f fit-image.its fit-image\n",
 		params.cmdname);
+	fprintf (stderr, "       %s -V ==> print version information and exit\n",
+		params.cmdname);
 
 	exit (EXIT_FAILURE);
 }