Skip to contentSkip to conversation

configure.ac: Don't search for c++ compiler

PATCH Applied Patrick Georgi 3 participants 16 messages Oct 2, 2014 – Nov 17, 2014 7 messages not in the archive Tegra architecture support

lore
Share & tools
lore

Patrick Georgi

author
#0 lore raw

PATCH 1/5 configure.ac: Don't search for c++ compiler

There is no C++ code to be compiled in the repository.

Signed-off-by Patrick Georgi <[email protected]>
---
 configure.ac | 1 -
 1 file changed, 1 deletion(-)
configure.ac +0 -1
index cc3999e..c720f93 100644
@@ -8,7 +8,6 @@ AC_CONFIG_SRCDIR([src/cbootimage.c])
 AC_CONFIG_HEADERS([config.h])
 
 # Checks for programs.
-AC_PROG_CXX
 AC_PROG_CC
 AM_PROG_CC_C_O
 PKG_PROG_PKG_CONFIG
Signature · 1 line
1.9.1

A message that is not in the archive (sent off-list or not yet received): 1412237788-20611-1-git-send-email-patrick-U1IrdZU7gScKt1dFYRw6Gg@public.gmane.org

author
#2 lore raw

PATCH 2/5 cbootimage: simplify code

Testing for e == 0 after exiting the function in any other case a couple of lines earlier is useless.

Found-by Coverity Scan
Signed-off-by Patrick Georgi <[email protected]>
---
 src/cbootimage.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
src/cbootimage.c +1 -1
index 5f746e3..62328b4 100644
@@ -211,7 +211,7 @@ main(int argc, char *argv[])
 
 	if (enable_debug) {
 		/* Debugging information... */
-		printf("bct size: %d\n", e == 0 ? context.bct_size : -1);
+		printf("bct size: %d\n", context.bct_size);
 	}
 
 	/* Open the raw output file. */
Signature · 1 line
1.9.1
author
#3 lore raw

PATCH 3/5 data_layout: improve memory handling

In reply to a message not in the archive

- free empty_blk if it's allocated and there's an error
- only free empty_blk if it's non-NULL. While POSIX requests such free()s to be safe, some implementations (eg Solaris) aren't compliant.

Found-by Coverity Scan
Signed-off-by Patrick Georgi <[email protected]>
---
 src/data_layout.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)
src/data_layout.c +4 -2
index 01f00ab..db0a0f0 100644
@@ -1004,12 +1004,14 @@ write_block_raw(build_image_context *context)
 		{
 			size_t bytes = pages_to_write * context->page_size;
 
-			if (fwrite(data, 1, bytes, context->raw_file) != bytes)
+			if (fwrite(data, 1, bytes, context->raw_file) != bytes) {
+				if (empty_blk) free(empty_blk);
 				return -1;
+			}
 		}
 	}
 
-	free(empty_blk);
+	if (empty_blk) free(empty_blk);
 	return 0;
 }
Signature · 1 line
1.9.1
author
#4 lore raw

PATCH 4/5 set: check seek success

In reply to a message not in the archive

This could silently fail which leads to surprising behaviour.

Found-by Coverity Scan
Signed-off-by Patrick Georgi <[email protected]>
---
 src/set.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)
src/set.c +5 -1
index ff32b53..907d640 100644
@@ -59,7 +59,11 @@ read_from_image(char	*filename,
 		return result;
 	}
 
-	fseek(fp, offset, SEEK_SET);
+	if (!fseek(fp, offset, SEEK_SET)) {
+		printf("Error: Couldn't seek to %s(%d)\n", filename, offset);
+		result = 1;
+		goto cleanup;
+	}
 
 	if (stat(filename, &stats) != 0) {
 		printf("Error: Unable to query info on bootloader path %s\n",
Signature · 1 line
1.9.1

A message that is not in the archive (sent off-list or not yet received): 1412237788-20611-4-git-send-email-patrick-U1IrdZU7gScKt1dFYRw6Gg@public.gmane.org

#6 lore raw

Re: [PATCH 4/5] set: check seek success

src/set.c read_from_image(char *filename, · line 62

Quoted 6 lines · Patrick Georgi

This could silently fail which leads to surprising behaviour.

src/set.c +1 -1
 
-	fseek(fp, offset, SEEK_SET);
+	if (!fseek(fp, offset, SEEK_SET)) {

fseek is supposed to return the current offset, or -1 on error. I think that should say if (fseek(...) < 0) or if (fseek(...) == -1)) or if (fseek(...) != offset).

A message that is not in the archive (sent off-list or not yet received): [email protected]

author
#8 lore raw

Re: [PATCH 4/5] set: check seek success

Am 03.10.2014 um 00:58 schrieb Stephen Warren:

Quoted 1 line

fseek is supposed to return the current offset, or -1 on error. I

The man page on my system disagrees: "Upon successful completion, fgetpos(), fseek(), fsetpos() return 0, and ftell() returns the current offset. Otherwise, -1 is returned and errno is set to indicate the error."

Patrick

A message that is not in the archive (sent off-list or not yet received): [email protected]

#10 lore raw

Re: [PATCH 4/5] set: check seek success

Quoted 4 lines · Patrick Georgi

Am 03.10.2014 um 00:58 schrieb Stephen Warren:

Quoted 1 line

fseek is supposed to return the current offset, or -1 on error. I

The man page on my system disagrees: "Upon successful completion, fgetpos(), fseek(), fsetpos() return 0, and ftell() returns the current offset. Otherwise, -1 is returned and errno is set to indicate the error."

That seems to exactly match what I said.

author
#11 lore raw

PATCH 5/5 data_layout: fail better on file access errors

In reply to a message not in the archive

fread could return only a partial result (eg. NVBOOT_CONFIG_TABLE_SIZE_MAX - 1 bytes), which right now would be accepted and only resolved by later code.

Found-by Coverity Scan
Signed-off-by Patrick Georgi <[email protected]>
---
 src/data_layout.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
src/data_layout.c +1 -1
index db0a0f0..11a0761 100644
@@ -1050,7 +1050,7 @@ int get_bct_size_from_image(build_image_context *context)
 	if (!fp)
 		return ENODATA;
 
-	if (!fread(buffer, 1, NVBOOT_CONFIG_TABLE_SIZE_MAX, fp)) {
+	if (fread(buffer, 1, NVBOOT_CONFIG_TABLE_SIZE_MAX, fp) != NVBOOT_CONFIG_TABLE_SIZE_MAX) {
 		fclose(fp);
 		return ENODATA;
 	}
Signature · 1 line
1.9.1
author
#13 lore raw

PATCH v2 4/5 set: check seek success

In reply to a message not in the archive

This could silently fail which leads to surprising behaviour.

Found-by Coverity Scan
Signed-off-by Patrick Georgi <[email protected]>
---
 src/set.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)
src/set.c +5 -1
index ff32b53..907d640 100644
@@ -59,7 +59,11 @@ read_from_image(char	*filename,
 		return result;
 	}
 
-	fseek(fp, offset, SEEK_SET);
+	if (fseek(fp, offset, SEEK_SET) == -1) {
+		printf("Error: Couldn't seek to %s(%d)\n", filename, offset);
+		result = 1;
+		goto cleanup;
+	}
 
 	if (stat(filename, &stats) != 0) {
 		printf("Error: Unable to query info on bootloader path %s\n",
Signature · 1 line
1.9.1

A message that is not in the archive (sent off-list or not yet received): [email protected]

#15 lore raw

Re: [PATCH 4/5 v2] set: check seek success

Quoted 7 lines · Patrick Georgi

This could silently fail which leads to surprising behaviour.

Found-by Coverity Scan
Signed-off-by Patrick Georgi <[email protected]>
---
 src/set.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

This clearly isn't a patch against the Linux kernel, so it'd be good to clarify what repository it should be applied to so people don't have to guess.

Thierry

A message that is not in the archive (sent off-list or not yet received): 14992150720.2771.879d79c6f3490faa837fa11cb97f42bf-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org

author
#17 lore raw

Re: [PATCH 4/5 v2] set: check seek success

Am 09.11.2014 um 02:04 schrieb Thierry Reding:

Quoted 9 lines · Patrick Georgi
Quoted 7 lines

This could silently fail which leads to surprising behaviour.

Found-by Coverity Scan
Signed-off-by Patrick Georgi <[email protected]>
---
 src/set.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)
Quoted 3 lines

This clearly isn't a patch against the Linux kernel, so it'd be good to clarify what repository it should be applied to so people don't have to guess.

Oops, I'm sorry. This is for https://github.com/NVIDIA/cbootimage

Thanks,
Patrick

Reviewed-by
#18 lore raw

Re: [PATCH 4/5 v2] set: check seek success

In reply to a message not in the archive
Quoted 13 lines · Patrick Georgi

This could silently fail which leads to surprising behaviour.

Found-by Coverity Scan
Signed-off-by Patrick Georgi <[email protected]>
---
 src/set.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)
src/set.c +0 -0
index ff32b53..907d640 100644

src/set.c read_from_image(char *filename, · line 63

Quoted 6 lines
src/set.c +2 -1
@@ -59,7 +59,11 @@ read_from_image(char	*filename,
 		return result;
 	}
 
-	fseek(fp, offset, SEEK_SET);
+	if (fseek(fp, offset, SEEK_SET) == -1) {
+		printf("Error: Couldn't seek to %s(%d)\n", filename, offset);

offset is unsigned, so the format specifier should be %u. I also wonder whether it would be good to output errno along with the message (or the string representation thereof) to increase the information content. But given that none of the other error messages have that it could be a separate patch.

With the %d -> %u for the format specifier, this is:

Reviewed-by Thierry Reding <[email protected]>
#19 lore raw

Re: [PATCH 4/5 v2] set: check seek success

Quoted 27 lines · Thierry Reding
Quoted 13 lines · Patrick Georgi

This could silently fail which leads to surprising behaviour.

Found-by Coverity Scan
Signed-off-by Patrick Georgi <[email protected]>
---
 src/set.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)
src/set.c +0 -0
index ff32b53..907d640 100644

src/set.c read_from_image(char *filename, · line 63

Quoted 6 lines
src/set.c +2 -1
@@ -59,7 +59,11 @@ read_from_image(char	*filename,
 		return result;
 	}
 
-	fseek(fp, offset, SEEK_SET);
+	if (fseek(fp, offset, SEEK_SET) == -1) {
+		printf("Error: Couldn't seek to %s(%d)\n", filename, offset);

offset is unsigned, so the format specifier should be %u. I also wonder whether it would be good to output errno along with the message (or the string representation thereof) to increase the information content. But given that none of the other error messages have that it could be a separate patch.

Quoted 3 lines

With the %d -> %u for the format specifier, this is:

Reviewed-by Thierry Reding <[email protected]>

I just realized that I have commit access to the cbootimage repository, so I just made the %d -> %u change myself and pushed.

Thanks,
Thierry

#20 lore raw

Re: [PATCH 4/5 v2] set: check seek success

In reply to Thierry Reding
Quoted 17 lines · Thierry Reding
Quoted 16 lines · Patrick Georgi

This could silently fail which leads to surprising behaviour.

Found-by Coverity Scan
Signed-off-by Patrick Georgi <[email protected]>
---
  src/set.c | 6 +++++-
  1 file changed, 5 insertions(+), 1 deletion(-)
src/set.c +0 -0
index ff32b53..907d640 100644
@@ -59,7 +59,11 @@ read_from_image(char	*filename,
  		return result;
  	}
Quoted 9 lines

src/set.c read_from_image(char *filename, · line 63

Quoted 3 lines
src/set.c +2 -1
-	fseek(fp, offset, SEEK_SET);
+	if (fseek(fp, offset, SEEK_SET) == -1) {
+		printf("Error: Couldn't seek to %s(%d)\n", filename, offset);

offset is unsigned, so the format specifier should be %u. I also wonder whether it would be good to output errno along with the message (or the string representation thereof) to increase the information content. But given that none of the other error messages have that it could be a separate patch.

Using perror() might be useful?

A message that is not in the archive (sent off-list or not yet received): [email protected]

#22 lore raw

Re: [PATCH 4/5 v2] set: check seek success

Quoted 28 lines · Stephen Warren
Quoted 17 lines · Thierry Reding
Quoted 16 lines · Patrick Georgi

This could silently fail which leads to surprising behaviour.

Found-by Coverity Scan
Signed-off-by Patrick Georgi <[email protected]>
---
 src/set.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)
src/set.c +0 -0
index ff32b53..907d640 100644
@@ -59,7 +59,11 @@ read_from_image(char	*filename,
 		return result;
 	}
Quoted 9 lines

src/set.c read_from_image(char *filename, · line 63

Quoted 3 lines
src/set.c +2 -1
-	fseek(fp, offset, SEEK_SET);
+	if (fseek(fp, offset, SEEK_SET) == -1) {
+		printf("Error: Couldn't seek to %s(%d)\n", filename, offset);

offset is unsigned, so the format specifier should be %u. I also wonder whether it would be good to output errno along with the message (or the string representation thereof) to increase the information content. But given that none of the other error messages have that it could be a separate patch.

Quoted 1 line

Using perror() might be useful?

Indeed, that would also make these messages go to stderr instead of stdout and be more idiomatic.

Thierry