Change in ...coreboot[master]: intelvbttool: Added support for reading vbt from sysfs

[Alex Feinman (Code Review)]

Alex Feinman has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/31531

Change subject: intelvbttool: Added support for reading vbt from sysfs
......................................................................

intelvbttool: Added support for reading vbt from sysfs

VBT on intel systems is available via sysfs as
    /sys/kernel/debug/dri/0/i915_vbt
However the size of this file reads as 0 causing
intelvbttool to fail. This patch implements incremental reads
with realloc for such cases.
After this patch is applied, intelvbttool can be used as follows:
    sudo intelvbttool -f /sys/kernel/debug/dri/0/i915_vbt -d

Change-Id: I5d17095a5747550b7115a54a7619b7294a846196
Signed-off-by: Alex Feinman 
---
M util/intelvbttool/intelvbttool.c
1 file changed, 21 insertions(+), 5 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/31/31531/1

diff --git a/util/intelvbttool/intelvbttool.c b/util/intelvbttool/intelvbttool.c
index 1444129..2941b42 100644
--- a/util/intelvbttool/intelvbttool.c
+++ b/util/intelvbttool/intelvbttool.c
@@ -29,6 +29,7 @@
 typedef uint16_t u16;
 typedef uint32_t u32;

+#define DEF_ALLOC 1024

typedef struct {
    u16 signature;
@@ -392,8 +393,8 @@
    	return NULL;
    }

-	const off_t size = ftell(fd);
-	if (size < 0 || size > SIZE_MAX) {
+	off_t read_size = ftell(fd);
+	if (read_size < 0 || read_size > SIZE_MAX) {
    	printerr("%s tell failed: %s\n", filename, strerror(errno));
    	fclose(fd);
    	return NULL;
@@ -405,14 +406,29 @@
    	return NULL;
    }

-	struct fileobject *fo = malloc_fo(size);
+	if ( read_size == 0 )
+		read_size = DEF_ALLOC;
+
+	struct fileobject *fo = malloc_fo(read_size);
    if (!fo) {
    	printerr("malloc failed\n");
    	fclose(fd);
    	return NULL;
    }

-	if (fread(fo->data, 1, size, fd) != size) {
+	off_t bytes_read = 0, cb;
+	while((cb = fread(fo->data + bytes_read, 1, read_size, fd)) > 0) {
+		bytes_read += cb;
+		struct fileobject* newfo = remalloc_fo(fo, fo->size + read_size);
+		if (!newfo) {
+			fclose(fd);
+			free_fo(fo);
+			return NULL;
+		}
+		fo = newfo;
+	}
+
+	if (!bytes_read) {
    	fclose(fd);
    	free_fo(fo);
    	return NULL;
@@ -424,7 +440,7 @@
    	return NULL;
    }

-	fo->size = size;
+	fo->size = bytes_read;

return fo;
 }

[build bot (Jenkins) (Code Review)]

build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31531 )

Change subject: intelvbttool: Added support for reading vbt from sysfs
......................................................................

Patch Set 1:

(5 comments)

https://review.coreboot.org/#/c/31531/1/util/intelvbttool/intelvbttool.c 
File util/intelvbttool/intelvbttool.c:

https://review.coreboot.org/#/c/31531/1/util/intelvbttool/intelvbttool.c@409 
PS1, Line 409: 	if ( read_size == 0 )
space prohibited after that open parenthesis '('

https://review.coreboot.org/#/c/31531/1/util/intelvbttool/intelvbttool.c@409 
PS1, Line 409: 	if ( read_size == 0 )
space prohibited before that close parenthesis ')'

https://review.coreboot.org/#/c/31531/1/util/intelvbttool/intelvbttool.c@420 
PS1, Line 420: 	while((cb = fread(fo->data + bytes_read, 1, read_size, fd)) > 0) {
space required before the open parenthesis '('

https://review.coreboot.org/#/c/31531/1/util/intelvbttool/intelvbttool.c@422 
PS1, Line 422: 		struct fileobject* newfo = remalloc_fo(fo, fo->size + read_size);
line over 80 characters

https://review.coreboot.org/#/c/31531/1/util/intelvbttool/intelvbttool.c@422 
PS1, Line 422: 		struct fileobject* newfo = remalloc_fo(fo, fo->size + read_size);
"foo* bar" should be "foo *bar"

[Alex Feinman (Code Review)]

Alex Feinman has uploaded a new patch set (#2). ( https://review.coreboot.org/c/coreboot/+/31531 )

Change subject: intelvbttool: Added support for reading vbt from sysfs
......................................................................

intelvbttool: Added support for reading vbt from sysfs

VBT on intel systems is available via sysfs as
    /sys/kernel/debug/dri/0/i915_vbt
However the size of this file reads as 0 causing
intelvbttool to fail. This patch implements incremental reads
with realloc for such cases.
After this patch is applied, intelvbttool can be used as follows:
    sudo intelvbttool -f /sys/kernel/debug/dri/0/i915_vbt -d

Change-Id: I5d17095a5747550b7115a54a7619b7294a846196
Signed-off-by: Alex Feinman 
---
M util/intelvbttool/intelvbttool.c
1 file changed, 21 insertions(+), 5 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/31/31531/2

[build bot (Jenkins) (Code Review)]

build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31531 )

Change subject: intelvbttool: Added support for reading vbt from sysfs
......................................................................

Patch Set 2:

(5 comments)

https://review.coreboot.org/#/c/31531/2/util/intelvbttool/intelvbttool.c 
File util/intelvbttool/intelvbttool.c:

https://review.coreboot.org/#/c/31531/2/util/intelvbttool/intelvbttool.c@409 
PS2, Line 409: 	if ( read_size == 0 )
space prohibited after that open parenthesis '('

https://review.coreboot.org/#/c/31531/2/util/intelvbttool/intelvbttool.c@409 
PS2, Line 409: 	if ( read_size == 0 )
space prohibited before that close parenthesis ')'

https://review.coreboot.org/#/c/31531/2/util/intelvbttool/intelvbttool.c@420 
PS2, Line 420: 	while((cb = fread(fo->data + bytes_read, 1, read_size, fd)) > 0) {
space required before the open parenthesis '('

https://review.coreboot.org/#/c/31531/2/util/intelvbttool/intelvbttool.c@422 
PS2, Line 422: 		struct fileobject* newfo = remalloc_fo(fo, fo->size + read_size);
line over 80 characters

https://review.coreboot.org/#/c/31531/2/util/intelvbttool/intelvbttool.c@422 
PS2, Line 422: 		struct fileobject* newfo = remalloc_fo(fo, fo->size + read_size);
"foo* bar" should be "foo *bar"

[Alex Feinman (Code Review)]

Hello build bot (Jenkins),

I'd like you to reexamine a change. Please visit

https://review.coreboot.org/c/coreboot/+/31531

to look at the new patch set (#3).

Change subject: intelvbttool: Added support for reading vbt from sysfs
......................................................................

intelvbttool: Added support for reading vbt from sysfs

VBT on intel systems is available via sysfs as
    /sys/kernel/debug/dri/0/i915_vbt
However the size of this file reads as 0 causing
intelvbttool to fail. This patch implements incremental reads
with realloc for such cases.
After this patch is applied, intelvbttool can be used as follows:
    sudo intelvbttool -f /sys/kernel/debug/dri/0/i915_vbt -d

Change-Id: I5d17095a5747550b7115a54a7619b7294a846196
Signed-off-by: Alex Feinman 
---
M util/intelvbttool/intelvbttool.c
1 file changed, 21 insertions(+), 5 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/31/31531/3

[build bot (Jenkins) (Code Review)]

build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31531 )

Change subject: intelvbttool: Added support for reading vbt from sysfs
......................................................................

Patch Set 3:

(1 comment)

https://review.coreboot.org/#/c/31531/3/util/intelvbttool/intelvbttool.c 
File util/intelvbttool/intelvbttool.c:

https://review.coreboot.org/#/c/31531/3/util/intelvbttool/intelvbttool.c@422 
PS3, Line 422: 		struct fileobject *newfo = remalloc_fo(fo, fo->size + read_size);
line over 80 characters

[Alex Feinman (Code Review)]

Alex Feinman has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31531 )

Change subject: intelvbttool: Added support for reading vbt from sysfs
......................................................................

Patch Set 3:

Nico,
I have modified intelvbttool as you suggested, to handle zero-length sysfs objects.
Since this is my first ever gerrit commit, I've bungled it a bit, but after 3 attempts I 
think I got it right

I'm sure you have tons of things to do on and off this project, but again, I don't quite know
what the etiquette of asking for review is, so feel free to bump it to someone else

[Patrick Rudolph (Code Review)]

Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31531 )

Change subject: intelvbttool: Added support for reading vbt from sysfs
......................................................................

Patch Set 3: Code-Review+1

(1 comment)

Tested. Works fine.

https://review.coreboot.org/#/c/31531/3/util/intelvbttool/intelvbttool.c 
File util/intelvbttool/intelvbttool.c:

https://review.coreboot.org/#/c/31531/3/util/intelvbttool/intelvbttool.c@409 
PS3, Line 409: 	if (read_size == 0)
add comment when this could happen. A regular user would wonder why we attempt to read a zero sized file.

[Paul Menzel (Code Review)]

Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31531 )

Change subject: intelvbttool: Added support for reading vbt from sysfs
......................................................................

Patch Set 3:

(1 comment)

https://review.coreboot.org/#/c/31531/3//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/#/c/31531/3//COMMIT_MSG@7 
PS3, Line 7: Added
Imperative mood: Add

[Nico Huber (Code Review)]

Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31531 )

Change subject: intelvbttool: Added support for reading vbt from sysfs
......................................................................

Patch Set 3: Code-Review+1

(2 comments)

Thanks for taking care of this. This could also be used
to implement reading from `stdin`. Though, I'm not sure
about a use case ;)

https://review.coreboot.org/#/c/31531/3/util/intelvbttool/intelvbttool.c 
File util/intelvbttool/intelvbttool.c:

https://review.coreboot.org/#/c/31531/3/util/intelvbttool/intelvbttool.c@409 
PS3, Line 409: 	if (read_size == 0)
> add comment when this could happen. […]

Or do something that doesn't require a comment:
Drop the fseek()/ftell() completely, and just use
the same code flow for all files.

With the remalloc_fo() call being implemented below
anyway, it would reduce the code and make me very
happy :)

https://review.coreboot.org/#/c/31531/3/util/intelvbttool/intelvbttool.c@422 
PS3, Line 422: 		struct fileobject *newfo = remalloc_fo(fo, fo->size + read_size);
This would realloc in any case, even if we knew the
file size initially and already allocated/read enough.
Which is ok, but reads oddly.

Change in ...coreboot[master]: intelvbttool: Add support for reading vbt from sysfs

[Alex Feinman (Code Review)]

Hello Patrick Rudolph, build bot (Jenkins), Nico Huber,

I'd like you to reexamine a change. Please visit

https://review.coreboot.org/c/coreboot/+/31531

to look at the new patch set (#4).

Change subject: intelvbttool: Add support for reading vbt from sysfs
......................................................................

intelvbttool: Add support for reading vbt from sysfs

VBT on Intel(R) systems is available via sysfs as
    /sys/kernel/debug/dri/0/i915_vbt
However the size of this file reads as 0 causing
intelvbttool to fail. This patch implements incremental reads
with realloc for such cases or whenever the file size is not
available (e.g. reading from stdin).
After this patch is applied, intelvbttool can be used as follows:
    sudo intelvbttool -f /sys/kernel/debug/dri/0/i915_vbt -d

Change-Id: I5d17095a5747550b7115a54a7619b7294a846196
Signed-off-by: Alex Feinman 
---
M util/intelvbttool/intelvbttool.c
1 file changed, 17 insertions(+), 22 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/31/31531/4

Change in ...coreboot[master]: intelvbttool: Add support for reading vbt from sysfs

[build bot (Jenkins) (Code Review)]

build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31531 )

Change subject: intelvbttool: Add support for reading vbt from sysfs
......................................................................

Patch Set 4:

(2 comments)

https://review.coreboot.org/#/c/31531/4/util/intelvbttool/intelvbttool.c 
File util/intelvbttool/intelvbttool.c:

https://review.coreboot.org/#/c/31531/4/util/intelvbttool/intelvbttool.c@399 
PS4, Line 399: 	while ((bytes_read = fread(fo->data + total_bytes_read, 1, read_size, fd)) > 0) {
line over 80 characters

https://review.coreboot.org/#/c/31531/4/util/intelvbttool/intelvbttool.c@401 
PS4, Line 401: 		struct fileobject *newfo = remalloc_fo(fo, fo->size + read_size);
line over 80 characters

Change in ...coreboot[master]: intelvbttool: Add support for reading vbt from sysfs

[Nico Huber (Code Review)]

Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31531 )

Change subject: intelvbttool: Add support for reading vbt from sysfs
......................................................................

Patch Set 4: Code-Review+2

Looks good to me, thanks.

Change in ...coreboot[master]: intelvbttool: Add support for reading vbt from sysfs

[Patrick Georgi (Code Review)]

Patrick Georgi has submitted this change and it was merged. ( https://review.coreboot.org/c/coreboot/+/31531 )

Change subject: intelvbttool: Add support for reading vbt from sysfs
......................................................................

intelvbttool: Add support for reading vbt from sysfs

VBT on Intel(R) systems is available via sysfs as
    /sys/kernel/debug/dri/0/i915_vbt
However the size of this file reads as 0 causing
intelvbttool to fail. This patch implements incremental reads
with realloc for such cases or whenever the file size is not
available (e.g. reading from stdin).
After this patch is applied, intelvbttool can be used as follows:
    sudo intelvbttool -f /sys/kernel/debug/dri/0/i915_vbt -d

Change-Id: I5d17095a5747550b7115a54a7619b7294a846196
Signed-off-by: Alex Feinman 
Reviewed-on: https://review.coreboot.org/c/31531
Tested-by: build bot (Jenkins) 
Reviewed-by: Nico Huber 
---
M util/intelvbttool/intelvbttool.c
1 file changed, 17 insertions(+), 22 deletions(-)

Approvals:
  build bot (Jenkins): Verified
  Nico Huber: Looks good to me, approved

diff --git a/util/intelvbttool/intelvbttool.c b/util/intelvbttool/intelvbttool.c
index 1444129..f8e4bda 100644
--- a/util/intelvbttool/intelvbttool.c
+++ b/util/intelvbttool/intelvbttool.c
@@ -29,6 +29,7 @@
 typedef uint16_t u16;
 typedef uint32_t u32;

+#define DEF_ALLOC 1024

typedef struct {
    u16 signature;
@@ -380,39 +381,33 @@
 static struct fileobject *read_file(const char *filename)
 {
    FILE *fd = fopen(filename, "rb");
+	off_t read_size = DEF_ALLOC;

if (!fd) {
    	printerr("%s open failed: %s\n", filename, strerror(errno));
    	return NULL;
    }

-	if (fseek(fd, 0, SEEK_END)) {
-		printerr("%s seek failed: %s\n", filename, strerror(errno));
-		fclose(fd);
-		return NULL;
-	}
-
-	const off_t size = ftell(fd);
-	if (size < 0 || size > SIZE_MAX) {
-		printerr("%s tell failed: %s\n", filename, strerror(errno));
-		fclose(fd);
-		return NULL;
-	}
-
-	if (fseek(fd, 0, SEEK_SET)) {
-		printerr("%s seek failed: %s\n", filename, strerror(errno));
-		fclose(fd);
-		return NULL;
-	}
-
-	struct fileobject *fo = malloc_fo(size);
+	struct fileobject *fo = malloc_fo(read_size);
    if (!fo) {
    	printerr("malloc failed\n");
    	fclose(fd);
    	return NULL;
    }

-	if (fread(fo->data, 1, size, fd) != size) {
+	off_t total_bytes_read = 0, bytes_read;
+	while ((bytes_read = fread(fo->data + total_bytes_read, 1, read_size, fd)) > 0) {
+		total_bytes_read += bytes_read;
+		struct fileobject *newfo = remalloc_fo(fo, fo->size + read_size);
+		if (!newfo) {
+			fclose(fd);
+			free_fo(fo);
+			return NULL;
+		}
+		fo = newfo;
+	}
+
+	if (!total_bytes_read) {
    	fclose(fd);
    	free_fo(fo);
    	return NULL;
@@ -424,7 +419,7 @@
    	return NULL;
    }

-	fo->size = size;
+	fo->size = total_bytes_read;

return fo;
 }