[U-Boot] [PATCH] mkimage: add "-V" option to print version information

newer
[U-Boot] [PATCH] Add support for...

Wolfgang Denk

11 Feb 2011 11 Feb '11

9:56 a.m.

Signed-off-by: Wolfgang Denk wd@denx.de --- tools/mkimage.c | 6 ++++++ 1 files changed, 6 insertions(+), 0 deletions(-)

diff --git a/tools/mkimage.c b/tools/mkimage.c index f5859d7..788484d 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,10 @@ main (int argc, char **argv) case 'v': params.vflag++; break; + case 'V': + printf("mkimage version %s\n", + U_BOOT_VERSION + 7); + exit(EXIT_SUCCESS); case 'x': params.xflag++; break; @@ -590,6 +595,7 @@ 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");

exit (EXIT_FAILURE); }

-- 1.7.4

Show replies by date

Wolfgang Denk

11 Feb 11 Feb

1:22 p.m.

New subject: [U-Boot] [PATCH v2] mkimage: add "-V" option to print version information

Signed-off-by: Wolfgang Denk wd@denx.de --- v2: fix missing argument to printf() call.

tools/mkimage.c | 7 +++++++ 1 files changed, 7 insertions(+), 0 deletions(-)

diff --git a/tools/mkimage.c b/tools/mkimage.c index f5859d7..127be57 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,10 @@ main (int argc, char **argv) case 'v': params.vflag++; break; + case 'V': + printf("mkimage version %s\n", + U_BOOT_VERSION + 7); + exit(EXIT_SUCCESS); case 'x': params.xflag++; break; @@ -590,6 +595,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); }

-- 1.7.4

Albert ARIBAUD

10:55 p.m.

New subject: [U-Boot] [PATCH v2] mkimage: add "-V" option to print version information

Wolfgang,

Please ignore my previous post on V1, I had not seen V2. My comment holds, though:

Le 11/02/2011 13:22, Wolfgang Denk a écrit :

...

Signed-off-by: Wolfgang Denkwd@denx.de

v2: fix missing argument to printf() call.

tools/mkimage.c | 7 +++++++ 1 files changed, 7 insertions(+), 0 deletions(-)

diff --git a/tools/mkimage.c b/tools/mkimage.c index f5859d7..127be57 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,10 @@ main (int argc, char **argv) case 'v': params.vflag++; break;
	case 'V':
		printf("mkimage version %s\n",
			U_BOOT_VERSION + 7);

If that magic number 7 (and the addition, as well) has any reason, it should at least be explained in a short comment.

...

		exit(EXIT_SUCCESS);
case 'x':
	params.xflag++;
	break;
@@ -590,6 +595,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); }

Amicalement,

-- Albert.

Wolfgang Denk

11:35 p.m.

New subject: [U-Boot] [PATCH v2] mkimage: add "-V" option to print version information

Dear Albert ARIBAUD,

In message 4D55B03A.9030602@free.fr you wrote:

...

Please ignore my previous post on V1, I had not seen V2. My comment holds, though:

...

...
	case 'V':
		printf("mkimage version %s\n",
			U_BOOT_VERSION + 7);
If that magic number 7 (and the addition, as well) has any reason, it > should at least be explained in a short comment.

Heh... I though I could leave this as an exercide for the reader ;-)

New version following...

Thanks for pointing out.

Best regards,

Wolfgang Denk

-- DENX Software Engineering GmbH, MD: Wolfgang Denk & Detlev Zundel HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd@denx.de "The hottest places in Hell are reserved for those who, in times of moral crisis, preserved their neutrality." - Dante

Albert ARIBAUD

10:52 p.m.

New subject: [U-Boot] [PATCH] mkimage: add "-V" option to print version information

Hi Wolfgang,

Le 11/02/2011 09:56, Wolfgang Denk a écrit :

...

Signed-off-by: Wolfgang Denkwd@denx.de

tools/mkimage.c | 6 ++++++ 1 files changed, 6 insertions(+), 0 deletions(-)

diff --git a/tools/mkimage.c b/tools/mkimage.c index f5859d7..788484d 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,10 @@ main (int argc, char **argv) case 'v': params.vflag++; break;
	case 'V':
		printf("mkimage version %s\n",
			U_BOOT_VERSION + 7);

That 7 is a pretty awful magic number here, isn't it? At least if there's a sane reason for this it should be summarized in a short comment.

...

		exit(EXIT_SUCCESS);
case 'x':
	params.xflag++;
	break;
@@ -590,6 +595,7 @@ 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");

exit (EXIT_FAILURE); }

Amicalement,

-- Albert.

Wolfgang Denk

11:41 p.m.

New subject: [U-Boot] [PATCH v3] mkimage: add "-V" option to print version information

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(-)

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); }

-- 1.7.4

Kim Phillips

12 Feb 12 Feb

12:11 a.m.

New subject: [U-Boot] [PATCH v3] mkimage: add "-V" option to print version information

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

7:47 a.m.

New subject: [U-Boot] [PATCH v3] mkimage: add "-V" option to print version information

Le 12/02/2011 00:11, Kim Phillips a écrit :

...

On Fri, 11 Feb 2011 23:41:43 +0100 Wolfgang Denkwd@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,

-- Albert.

Wolfgang Denk

10:29 a.m.

New subject: [U-Boot] [PATCH v3] mkimage: add "-V" option to print version information

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

Kim Phillips

11:04 p.m.

New subject: [U-Boot] [PATCH v3] mkimage: add "-V" option to print version information

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]

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

Kim

Wolfgang Denk

10:37 a.m.

New subject: [U-Boot] [PATCH v3] mkimage: add "-V" option to print version information

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

11:17 p.m.

New subject: [U-Boot] [PATCH v3] mkimage: add "-V" option to print version information

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

13 Feb 13 Feb

12:31 a.m.

New subject: [U-Boot] [PATCH v3] mkimage: add "-V" option to print version information

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

-- DENX Software Engineering GmbH, MD: Wolfgang Denk & Detlev Zundel HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd@denx.de Ill-chosen abstraction is particularly evident in the design of the ADA runtime system. The interface to the ADA runtime system is so opaque that it is impossible to model or predict its performance, making it effectively useless for real-time systems. - Marc D. Donner and David H. Jameson.

Wolfgang Denk

12 Feb 12 Feb

10:37 a.m.

New subject: [U-Boot] [PATCH v4] mkimage: add "-V" option to print version information

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 v3: avoid offset into U_BOOT_VERSION string completely and define a new PLAIN_VERSION variable instead; this has the benefit that it can be used in other places as well where such version information might be needed (fw_{set,print}env etc. comes to mind).

Makefile | 8 ++++++-- tools/mkimage.c | 6 ++++++ 2 files changed, 12 insertions(+), 2 deletions(-)

diff --git a/Makefile b/Makefile index 05b404d..8721b59 100644 --- a/Makefile +++ b/Makefile @@ -414,8 +414,12 @@ $(U_BOOT_ONENAND): $(ONENAND_IPL) $(obj)u-boot.bin cat $(ONENAND_BIN) $(obj)u-boot.bin > $(obj)u-boot-onenand.bin

$(VERSION_FILE): - @( printf '#define U_BOOT_VERSION "U-Boot %s%s"\n' "$(U_BOOT_VERSION)" \ - '$(shell $(TOPDIR)/tools/setlocalversion $(TOPDIR))' ) > $@.tmp + @( localvers='$(shell $(TOPDIR)/tools/setlocalversion $(TOPDIR))' ; \ + printf '#define PLAIN_VERSION "%s%s"\n' \ + "$(U_BOOT_VERSION)" "$${localvers}" ; \ + printf '#define U_BOOT_VERSION "U-Boot %s%s"\n' \ + "$(U_BOOT_VERSION)" "$${localvers}" ; \ + ) > $@.tmp @( printf '#define CC_VERSION_STRING "%s"\n' \ '$(shell $(CC) --version | head -n 1)' )>> $@.tmp @( printf '#define LD_VERSION_STRING "%s"\n' \ diff --git a/tools/mkimage.c b/tools/mkimage.c index f5859d7..60f7263 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,9 @@ main (int argc, char **argv) case 'v': params.vflag++; break; + case 'V': + printf("mkimage version %s\n", PLAIN_VERSION); + exit(EXIT_SUCCESS); case 'x': params.xflag++; break; @@ -590,6 +594,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); }

-- 1.7.4

Kim Phillips

13 Feb 13 Feb

12:13 a.m.

New subject: [U-Boot] [PATCH v4] mkimage: add "-V" option to print version information

On Sat, 12 Feb 2011 10:37:11 +0100 Wolfgang Denk wd@denx.de wrote:

...

@( printf '#define U_BOOT_VERSION "U-Boot %s%s"\n' "$(U_BOOT_VERSION)" \

 '$(shell $(TOPDIR)/tools/setlocalversion $(TOPDIR))' ) > $@.tmp

@( localvers='$(shell $(TOPDIR)/tools/setlocalversion $(TOPDIR))' ; \

   printf '#define PLAIN_VERSION "%s%s"\n' \

   	"$(U_BOOT_VERSION)" "$${localvers}" ; \

   printf '#define U_BOOT_VERSION "U-Boot %s%s"\n' \

   	"$(U_BOOT_VERSION)" "$${localvers}" ; \

```
) > $@.tmp
```

IMO, PLAIN_VERSION isn't descriptive enough (should really be called VERSION..?). How about going with something like:

#define U_BOOT_STR "U-Boot" #define U_BOOT_VERSION U_BOOT_STR " %s%s"...

and then

...

```
	case 'V':
```

		printf("mkimage version %s\n", PLAIN_VERSION);

```
		exit(EXIT_SUCCESS);
```

&U_BOOT_VERSION[sizeof(U_BOOT_STR)]

(the - 1 is not necessary since we want to include the ' ')

this maintains consistency and the fact that the mkimage version is directly tied to it's parent project, U-Boot's, version number.

Kim

Wolfgang Denk

12:35 a.m.

New subject: [U-Boot] [PATCH v4] mkimage: add "-V" option to print version information

Dear Kim Phillips,

In message 20110212171349.f0f5d472.kim.phillips@freescale.com you wrote:

...

@( printf '#define U_BOOT_VERSION "U-Boot %s%s"\n' "$(U_BOOT_VERSION)" \

 '$(shell $(TOPDIR)/tools/setlocalversion $(TOPDIR))' ) > $@.tmp

@( localvers='$(shell $(TOPDIR)/tools/setlocalversion $(TOPDIR))' ; \

   printf '#define PLAIN_VERSION "%s%s"\n' \

   	"$(U_BOOT_VERSION)" "$${localvers}" ; \

   printf '#define U_BOOT_VERSION "U-Boot %s%s"\n' \

   	"$(U_BOOT_VERSION)" "$${localvers}" ; \

```
) > $@.tmp
```

IMO, PLAIN_VERSION isn't descriptive enough (should really be called VERSION..?). How about going with something like:

#define U_BOOT_STR "U-Boot" #define U_BOOT_VERSION U_BOOT_STR " %s%s"...

No - not unless you guarantee that this syntax is compatible with all assemblers that may be used to build U-Boot.

...

and then

...
	case 'V':
		printf("mkimage version %s\n", PLAIN_VERSION);
		exit(EXIT_SUCCESS);
&U_BOOT_VERSION[sizeof(U_BOOT_STR)]

(the - 1 is not necessary since we want to include the ' ')

No again. This is, um, ugly, and completely unnecessary.

Best regards,

Wolfgang Denk

-- DENX Software Engineering GmbH, MD: Wolfgang Denk & Detlev Zundel HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd@denx.de Man is the best computer we can put aboard a spacecraft ... and the only one that can be mass produced with unskilled labor. - Wernher von Braun

Kim Phillips

1:08 a.m.

New subject: [U-Boot] [PATCH v4] mkimage: add "-V" option to print version information

On Sun, 13 Feb 2011 00:35:08 +0100 Wolfgang Denk wd@denx.de wrote:

...

Dear Kim Phillips,

In message 20110212171349.f0f5d472.kim.phillips@freescale.com you wrote:

...
...
@( printf '#define U_BOOT_VERSION "U-Boot %s%s"\n' "$(U_BOOT_VERSION)" \
 '$(shell $(TOPDIR)/tools/setlocalversion $(TOPDIR))' ) > $@.tmp
@( localvers='$(shell $(TOPDIR)/tools/setlocalversion $(TOPDIR))' ; \
   printf '#define PLAIN_VERSION "%s%s"\n' \
   	"$(U_BOOT_VERSION)" "$${localvers}" ; \
   printf '#define U_BOOT_VERSION "U-Boot %s%s"\n' \
   	"$(U_BOOT_VERSION)" "$${localvers}" ; \
) > $@.tmp
IMO, PLAIN_VERSION isn't descriptive enough (should really be called VERSION..?). How about going with something like:

#define U_BOOT_STR "U-Boot" #define U_BOOT_VERSION U_BOOT_STR " %s%s"...
No - not unless you guarantee that this syntax is compatible with all assemblers that may be used to build U-Boot.

I cannot do that, but, ok, not such a big deal:

/* prefix lengths must match */ #define U_BOOT_STR "U-Boot" #define U_BOOT_VERSION "U-Boot %s%s"...

...

...
...
	case 'V':
		printf("mkimage version %s\n", PLAIN_VERSION);
		exit(EXIT_SUCCESS);
&U_BOOT_VERSION[sizeof(U_BOOT_STR)]

(the - 1 is not necessary since we want to include the ' ')
No again. This is, um, ugly, and completely unnecessary.

that's a matter of personal taste, but IMO it's better than PLAIN_VERSION (what's that? - VERSION_NUMBERONLY would be way more descriptive).

If it's the &..[..] that's not appealing, feel free to do as the '+ 7' code but as '+ sizeof(...)' (to maintain a not-so-completely unnecessary clarity & consistency..but that's my opinion).

Kim

Wolfgang Denk

12 Apr 12 Apr

10:37 p.m.

New subject: [U-Boot] [PATCH v4] mkimage: add "-V" option to print version information

Dear Wolfgang Denk,

In message 1297503431-19435-1-git-send-email-wd@denx.de you wrote:

...

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 v3: avoid offset into U_BOOT_VERSION string completely and define a new PLAIN_VERSION variable instead; this has the benefit that it can be used in other places as well where such version information might be needed (fw_{set,print}env etc. comes to mind).

Makefile | 8 ++++++-- tools/mkimage.c | 6 ++++++ 2 files changed, 12 insertions(+), 2 deletions(-)

Applied.

Best regards,

Wolfgang Denk

5138

Age (days ago)

5198

Last active (days ago)

List overview

Download

17 comments

3 participants

tags (0)

participants (3)

Albert ARIBAUD
Kim Phillips
Wolfgang Denk