Change in coreboot[master]: [RFC] vpd: Add vpd_get_int() function

[Nico Huber (Code Review)]

Hello Martin Roth, Marc Jones, Johnny Lin, Angel Pons,

I'd like you to do a code review. Please visit

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

to review the following change.

Change subject: [RFC] vpd: Add vpd_get_int() function
......................................................................

[RFC] vpd: Add vpd_get_int() function

Change-Id: I1c1b5710a5236fe4a3bdda1fc978393e636e9817
Signed-off-by: Nico Huber 
---
M src/drivers/vpd/vpd.c
M src/drivers/vpd/vpd.h
2 files changed, 32 insertions(+), 0 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/73/45773/1

diff --git a/src/drivers/vpd/vpd.c b/src/drivers/vpd/vpd.c
index c332a6e..d3ff370 100644
--- a/src/drivers/vpd/vpd.c
+++ b/src/drivers/vpd/vpd.c
@@ -3,10 +3,12 @@
 #include <assert.h>
 #include <console/console.h>
 #include <cbmem.h>
+#include <ctype.h>
 #include <fmap.h>
 #include <program_loading.h>
 #include <string.h>
 #include <timestamp.h>
+#include <types.h>

#include "vpd.h"
 #include "vpd_decode.h"
@@ -274,4 +276,25 @@
    	return false;
 }

+/*
+ * Find value of integer type by vpd key.
+ *
+ * Expects to find a decimal string, trailing chars are ignored.
+ * Returns true if the key is found and the value is not too long and
+ * starts with a decimal digit. Leaves `val` untouched if unsuccessful.
+ */
+bool vpd_get_int(const char *const key, const enum vpd_region region, int *const val)
+{
+	char value[11];
+
+	if (!vpd_gets(key, value, sizeof(value), region))
+		return false;
+
+	if (!isdigit(*value))
+		return false;
+
+	*val = (int)atol(value);
+	return true;
+}
+
 ROMSTAGE_CBMEM_INIT_HOOK(cbmem_add_cros_vpd)
diff --git a/src/drivers/vpd/vpd.h b/src/drivers/vpd/vpd.h
index 25e0aed..817867a 100644
--- a/src/drivers/vpd/vpd.h
+++ b/src/drivers/vpd/vpd.h
@@ -50,4 +50,13 @@
 bool vpd_get_bool(const char *key, enum vpd_region region,
    uint8_t *val);

+/*
+ * Find value of integer type by vpd key.
+ *
+ * Expects to find a decimal string, trailing chars are ignored.
+ * Returns true if the key is found and the value is not too long and
+ * starts with a decimal digit.
+ */
+bool vpd_get_int(const char *key, enum vpd_region region, int *val);
+
 #endif  /* __VPD_H__ */

[Angel Pons (Code Review)]

Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45773 )

Change subject: [RFC] vpd: Add vpd_get_int() function
......................................................................

Patch Set 1: Code-Review+1

[Julius Werner (Code Review)]

Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45773 )

Change subject: [RFC] vpd: Add vpd_get_int() function
......................................................................

Patch Set 3:

(1 comment)

https://review.coreboot.org/c/coreboot/+/45773/3/src/drivers/vpd/vpd.c 
File src/drivers/vpd/vpd.c:

https://review.coreboot.org/c/coreboot/+/45773/3/src/drivers/vpd/vpd.c@290 
PS3, Line 290: 	if (!vpd_gets(key, value, sizeof(value), region))
Isn't this easier/cleaner if you build it on top of vpd_find() (like vpd_get_bool())?

[Nico Huber (Code Review)]

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

Change subject: [RFC] vpd: Add vpd_get_int() function
......................................................................

Patch Set 3:

(1 comment)

https://review.coreboot.org/c/coreboot/+/45773/3/src/drivers/vpd/vpd.c 
File src/drivers/vpd/vpd.c:

https://review.coreboot.org/c/coreboot/+/45773/3/src/drivers/vpd/vpd.c@290 
PS3, Line 290: 	if (!vpd_gets(key, value, sizeof(value), region))
> Isn't this easier/cleaner if you build it on top of vpd_find() (like vpd_get_bool())?

Are VPDs guaranteed to be 0-terminated?

[Julius Werner (Code Review)]

Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45773 )

Change subject: [RFC] vpd: Add vpd_get_int() function
......................................................................

Patch Set 3:

(1 comment)

https://review.coreboot.org/c/coreboot/+/45773/3/src/drivers/vpd/vpd.c 
File src/drivers/vpd/vpd.c:

https://review.coreboot.org/c/coreboot/+/45773/3/src/drivers/vpd/vpd.c@290 
PS3, Line 290: 	if (!vpd_gets(key, value, sizeof(value), region))
> Are VPDs guaranteed to be 0-terminated?

Fair enough. (I think the 'type' byte that would follow for the next entry or the terminator cannot be a valid digit character, but you're right it's probably best not to rely on that.)

[Julius Werner (Code Review)]

Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45773 )

Change subject: [RFC] vpd: Add vpd_get_int() function
......................................................................

Patch Set 3: Code-Review+2

[Angel Pons (Code Review)]

Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45773 )

Change subject: [RFC] vpd: Add vpd_get_int() function
......................................................................

Patch Set 4: Code-Review+2

(1 comment)

https://review.coreboot.org/c/coreboot/+/45773/4//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/c/coreboot/+/45773/4//COMMIT_MSG@7 
PS4, Line 7: [RFC]
Drop `[RFC] `

Change in coreboot[master]: vpd: Add vpd_get_int() function

[Nico Huber (Code Review)]

Hello build bot (Jenkins), Martin Roth, Jonathan Zhang, Marc Jones, Johnny Lin, insomniac, Angel Pons, Julius Werner,

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

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

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

Change subject: vpd: Add vpd_get_int() function
......................................................................

vpd: Add vpd_get_int() function

Change-Id: I1c1b5710a5236fe4a3bdda1fc978393e636e9817
Signed-off-by: Nico Huber 
---
M src/drivers/vpd/vpd.c
M src/drivers/vpd/vpd.h
2 files changed, 32 insertions(+), 0 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/73/45773/5

Change in coreboot[master]: vpd: Add vpd_get_int() function

[Nico Huber (Code Review)]

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

Change subject: vpd: Add vpd_get_int() function
......................................................................

Patch Set 5:

(1 comment)

https://review.coreboot.org/c/coreboot/+/45773/4//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/c/coreboot/+/45773/4//COMMIT_MSG@7 
PS4, Line 7: [RFC]
> Drop `[RFC] `

Done

Change in coreboot[master]: vpd: Add vpd_get_int() function

[Nico Huber (Code Review)]

Nico Huber has submitted this change. ( https://review.coreboot.org/c/coreboot/+/45773 )

Change subject: vpd: Add vpd_get_int() function
......................................................................

vpd: Add vpd_get_int() function

Change-Id: I1c1b5710a5236fe4a3bdda1fc978393e636e9817
Signed-off-by: Nico Huber 
Reviewed-on: https://review.coreboot.org/c/coreboot/+/45773
Reviewed-by: Angel Pons 
Reviewed-by: Julius Werner 
Tested-by: build bot (Jenkins) 
---
M src/drivers/vpd/vpd.c
M src/drivers/vpd/vpd.h
2 files changed, 32 insertions(+), 0 deletions(-)

Approvals:
  build bot (Jenkins): Verified
  Julius Werner: Looks good to me, approved
  Angel Pons: Looks good to me, approved

diff --git a/src/drivers/vpd/vpd.c b/src/drivers/vpd/vpd.c
index c332a6e..d3ff370 100644
--- a/src/drivers/vpd/vpd.c
+++ b/src/drivers/vpd/vpd.c
@@ -3,10 +3,12 @@
 #include <assert.h>
 #include <console/console.h>
 #include <cbmem.h>
+#include <ctype.h>
 #include <fmap.h>
 #include <program_loading.h>
 #include <string.h>
 #include <timestamp.h>
+#include <types.h>

#include "vpd.h"
 #include "vpd_decode.h"
@@ -274,4 +276,25 @@
    	return false;
 }

+/*
+ * Find value of integer type by vpd key.
+ *
+ * Expects to find a decimal string, trailing chars are ignored.
+ * Returns true if the key is found and the value is not too long and
+ * starts with a decimal digit. Leaves `val` untouched if unsuccessful.
+ */
+bool vpd_get_int(const char *const key, const enum vpd_region region, int *const val)
+{
+	char value[11];
+
+	if (!vpd_gets(key, value, sizeof(value), region))
+		return false;
+
+	if (!isdigit(*value))
+		return false;
+
+	*val = (int)atol(value);
+	return true;
+}
+
 ROMSTAGE_CBMEM_INIT_HOOK(cbmem_add_cros_vpd)
diff --git a/src/drivers/vpd/vpd.h b/src/drivers/vpd/vpd.h
index 25e0aed..817867a 100644
--- a/src/drivers/vpd/vpd.h
+++ b/src/drivers/vpd/vpd.h
@@ -50,4 +50,13 @@
 bool vpd_get_bool(const char *key, enum vpd_region region,
    uint8_t *val);

+/*
+ * Find value of integer type by vpd key.
+ *
+ * Expects to find a decimal string, trailing chars are ignored.
+ * Returns true if the key is found and the value is not too long and
+ * starts with a decimal digit.
+ */
+bool vpd_get_int(const char *key, enum vpd_region region, int *val);
+
 #endif  /* __VPD_H__ */