Attention is currently required from: Sean Rhodes, Martin Roth, Jakub Czapiga, Matt DeVillier, Angel Pons, Arthur Heymans, Patrick Rudolph. Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/52564 )
Change subject: drivers/efi: Add EFI variable store option support ......................................................................
Patch Set 10:
(5 comments)
File src/drivers/efi/Kconfig:
https://review.coreboot.org/c/coreboot/+/52564/comment/780bbf4d_9d7e8a55 PS10, Line 1: DRIVERS_EFI_VARIABLE_STORE
If it only needs the headers, why not include them directly? I'd avoid binding to a specific edk2 ve […]
yes, it needs to bind against UDK as the Makefile.inc sets the header include directory. Are there conflicts if I select multiple UDK at the same time?
File src/drivers/efi/efivars.c:
https://review.coreboot.org/c/coreboot/+/52564/comment/9e2432fe_2be390d5 PS10, Line 75: WA
?
W=WCHAR A=ASCII typically used on windows. Any better idea?
https://review.coreboot.org/c/coreboot/+/52564/comment/c257150c_49752c74 PS10, Line 83: while (1)
I'd prefer a for-loop to avoid going past the end of `msg`
As long as both strings are NULL termintated I don't see an issue.
https://review.coreboot.org/c/coreboot/+/52564/comment/3b204f4f_6c0d7cdb PS10, Line 86: (r = (c - msg[i]))
Is it necessary to do this assignment in the condition?
I just copied this code from src/lib.
https://review.coreboot.org/c/coreboot/+/52564/comment/3f25ac60_cfc1f317 PS10, Line 271: void *
Why is this a void pointer?
The third argument of walk_variables has the function prototype: ``` cb_err_t (*walker)(struct region_device *rdev, VARIABLE_HEADER *hdr, size_t hdr_size, void *arg, bool *stop) ```
-
Patrick Rudolph (Code Review)