Hi Mattijs, On Thu, Feb 8, 2024 at 3:00 PM Mattijs Korpershoek <mkorpersh...@baylibre.com> wrote: > > Hi Igor, > > Thank you for the patch. > > On mar., févr. 06, 2024 at 23:31, Igor Opaniuk <igor.opan...@foundries.io> > wrote: > > > From: Igor Opaniuk <igor.opan...@gmail.com> > > > > Introduce AVB_OPS_CHECK macro for checking AvbOps before using > > it to avoid code duplication. > > Simplify and add more context for prints where it's needed. > > > > Signed-off-by: Igor Opaniuk <igor.opan...@gmail.com> > > --- > > > > cmd/avb.c | 156 ++++++++++++++++++++++++++++-------------------------- > > 1 file changed, 80 insertions(+), 76 deletions(-) > > > > diff --git a/cmd/avb.c b/cmd/avb.c > > index ce8b63873f2..ae0012c0e79 100644 > > --- a/cmd/avb.c > > +++ b/cmd/avb.c > > @@ -11,6 +11,14 @@ > > #include <mmc.h> > > > > #define AVB_BOOTARGS "avb_bootargs" > > + > > +#define AVB_OPS_CHECK(avb_ops) do { \ > > + if (!(avb_ops)) { \ > > + printf("AVB is not initialized, please run 'avb init > > <id>'\n"); \ > > + return CMD_RET_FAILURE; \ > > + } \ > > +} while (false) > > + > checkpatch.pl --u-boot seems to complain about this: > WARNING: Macros with flow control statements should be avoided > #25: FILE: cmd/avb.c:15: > > This seems documented in the kernel coding style as well. > https://www.kernel.org/doc/html/latest/process/coding-style.html#macros-enums-and-rtl > > I'd argue that in this case, it's not better or worse in terme of > readability but i'd prefer to stick to the rules in this case. > > The error handling (using ret) and the better prints seem fine though. > Could we drop the AVB_OPS_CHECK macro for v2? Yeah, I was just trying to find a tradeoff between readability/code duplication and coding style.
Will drop it in v2. Thanks! > > > static struct AvbOps *avb_ops; > > > > int do_avb_init(struct cmd_tbl *cmdtp, int flag, int argc, char *const > > argv[]) > > @@ -28,8 +36,10 @@ int do_avb_init(struct cmd_tbl *cmdtp, int flag, int > > argc, char *const argv[]) > > avb_ops = avb_ops_alloc(mmc_dev); > > if (avb_ops) > > return CMD_RET_SUCCESS; > > + else > > + printf("Can't allocate AvbOps"); > > > > - printf("Failed to initialize avb2\n"); > > + printf("Failed to initialize AVB\n"); > > > > return CMD_RET_FAILURE; > > } > > @@ -41,11 +51,9 @@ int do_avb_read_part(struct cmd_tbl *cmdtp, int flag, > > int argc, > > s64 offset; > > size_t bytes, bytes_read = 0; > > void *buffer; > > + int ret; > > > > - if (!avb_ops) { > > - printf("AVB 2.0 is not initialized, please run 'avb init'\n"); > > - return CMD_RET_USAGE; > > - } > > + AVB_OPS_CHECK(avb_ops); > > > > if (argc != 5) > > return CMD_RET_USAGE; > > @@ -55,14 +63,15 @@ int do_avb_read_part(struct cmd_tbl *cmdtp, int flag, > > int argc, > > bytes = hextoul(argv[3], NULL); > > buffer = (void *)hextoul(argv[4], NULL); > > > > - if (avb_ops->read_from_partition(avb_ops, part, offset, bytes, > > - buffer, &bytes_read) == > > - AVB_IO_RESULT_OK) { > > + ret = avb_ops->read_from_partition(avb_ops, part, offset, > > + bytes, buffer, &bytes_read); > > + if (ret == AVB_IO_RESULT_OK) { > > printf("Read %zu bytes\n", bytes_read); > > return CMD_RET_SUCCESS; > > } > > > > - printf("Failed to read from partition\n"); > > + printf("Failed to read from partition '%s', err = %d\n", > > + part, ret); > > > > return CMD_RET_FAILURE; > > } > > @@ -74,11 +83,9 @@ int do_avb_read_part_hex(struct cmd_tbl *cmdtp, int > > flag, int argc, > > s64 offset; > > size_t bytes, bytes_read = 0; > > char *buffer; > > + int ret; > > > > - if (!avb_ops) { > > - printf("AVB 2.0 is not initialized, please run 'avb init'\n"); > > - return CMD_RET_USAGE; > > - } > > + AVB_OPS_CHECK(avb_ops); > > > > if (argc != 4) > > return CMD_RET_USAGE; > > @@ -94,8 +101,9 @@ int do_avb_read_part_hex(struct cmd_tbl *cmdtp, int > > flag, int argc, > > } > > memset(buffer, 0, bytes); > > > > - if (avb_ops->read_from_partition(avb_ops, part, offset, bytes, buffer, > > - &bytes_read) == AVB_IO_RESULT_OK) { > > + ret = avb_ops->read_from_partition(avb_ops, part, offset, > > + bytes, buffer, &bytes_read); > > + if (ret == AVB_IO_RESULT_OK) { > > printf("Requested %zu, read %zu bytes\n", bytes, bytes_read); > > printf("Data: "); > > for (int i = 0; i < bytes_read; i++) > > @@ -107,7 +115,8 @@ int do_avb_read_part_hex(struct cmd_tbl *cmdtp, int > > flag, int argc, > > return CMD_RET_SUCCESS; > > } > > > > - printf("Failed to read from partition\n"); > > + printf("Failed to read from partition '%s', err = %d\n", > > + part, ret); > > > > free(buffer); > > return CMD_RET_FAILURE; > > @@ -120,11 +129,9 @@ int do_avb_write_part(struct cmd_tbl *cmdtp, int flag, > > int argc, > > s64 offset; > > size_t bytes; > > void *buffer; > > + int ret; > > > > - if (!avb_ops) { > > - printf("AVB 2.0 is not initialized, run 'avb init' first\n"); > > - return CMD_RET_FAILURE; > > - } > > + AVB_OPS_CHECK(avb_ops); > > > > if (argc != 5) > > return CMD_RET_USAGE; > > @@ -134,13 +141,15 @@ int do_avb_write_part(struct cmd_tbl *cmdtp, int > > flag, int argc, > > bytes = hextoul(argv[3], NULL); > > buffer = (void *)hextoul(argv[4], NULL); > > > > - if (avb_ops->write_to_partition(avb_ops, part, offset, bytes, buffer) > > == > > - AVB_IO_RESULT_OK) { > > + ret = avb_ops->write_to_partition(avb_ops, part, offset, > > + bytes, buffer); > > + if (ret == AVB_IO_RESULT_OK) { > > printf("Wrote %zu bytes\n", bytes); > > return CMD_RET_SUCCESS; > > } > > > > - printf("Failed to write in partition\n"); > > + printf("Failed to write in partition '%s', err = %d\n", > > + part, ret); > > > > return CMD_RET_FAILURE; > > } > > @@ -150,24 +159,23 @@ int do_avb_read_rb(struct cmd_tbl *cmdtp, int flag, > > int argc, > > { > > size_t index; > > u64 rb_idx; > > + int ret; > > > > - if (!avb_ops) { > > - printf("AVB 2.0 is not initialized, run 'avb init' first\n"); > > - return CMD_RET_FAILURE; > > - } > > + AVB_OPS_CHECK(avb_ops); > > > > if (argc != 2) > > return CMD_RET_USAGE; > > > > index = (size_t)hextoul(argv[1], NULL); > > > > - if (avb_ops->read_rollback_index(avb_ops, index, &rb_idx) == > > - AVB_IO_RESULT_OK) { > > + ret = avb_ops->read_rollback_index(avb_ops, index, &rb_idx); > > + if (ret == AVB_IO_RESULT_OK) { > > printf("Rollback index: %llx\n", rb_idx); > > return CMD_RET_SUCCESS; > > } > > > > - printf("Failed to read rollback index\n"); > > + printf("Failed to read rollback index id = %zu, err = %d\n", > > + index, ret); > > > > return CMD_RET_FAILURE; > > } > > @@ -177,11 +185,9 @@ int do_avb_write_rb(struct cmd_tbl *cmdtp, int flag, > > int argc, > > { > > size_t index; > > u64 rb_idx; > > + int ret; > > > > - if (!avb_ops) { > > - printf("AVB 2.0 is not initialized, run 'avb init' first\n"); > > - return CMD_RET_FAILURE; > > - } > > + AVB_OPS_CHECK(avb_ops); > > > > if (argc != 3) > > return CMD_RET_USAGE; > > @@ -189,11 +195,12 @@ int do_avb_write_rb(struct cmd_tbl *cmdtp, int flag, > > int argc, > > index = (size_t)hextoul(argv[1], NULL); > > rb_idx = hextoul(argv[2], NULL); > > > > - if (avb_ops->write_rollback_index(avb_ops, index, rb_idx) == > > - AVB_IO_RESULT_OK) > > + ret = avb_ops->write_rollback_index(avb_ops, index, rb_idx); > > + if (ret == AVB_IO_RESULT_OK) > > return CMD_RET_SUCCESS; > > > > - printf("Failed to write rollback index\n"); > > + printf("Failed to write rollback index id = %zu, err = %d\n", > > + index, ret); > > > > return CMD_RET_FAILURE; > > } > > @@ -203,25 +210,25 @@ int do_avb_get_uuid(struct cmd_tbl *cmdtp, int flag, > > { > > const char *part; > > char buffer[UUID_STR_LEN + 1]; > > + int ret; > > > > - if (!avb_ops) { > > - printf("AVB 2.0 is not initialized, run 'avb init' first\n"); > > - return CMD_RET_FAILURE; > > - } > > + AVB_OPS_CHECK(avb_ops); > > > > if (argc != 2) > > return CMD_RET_USAGE; > > > > part = argv[1]; > > > > - if (avb_ops->get_unique_guid_for_partition(avb_ops, part, buffer, > > - UUID_STR_LEN + 1) == > > - AVB_IO_RESULT_OK) { > > + ret = avb_ops->get_unique_guid_for_partition(avb_ops, part, > > + buffer, > > + UUID_STR_LEN + 1); > > + if (ret == AVB_IO_RESULT_OK) { > > printf("'%s' UUID: %s\n", part, buffer); > > return CMD_RET_SUCCESS; > > } > > > > - printf("Failed to read UUID\n"); > > + printf("Failed to read partition '%s' UUID, err = %d\n", > > + part, ret); > > > > return CMD_RET_FAILURE; > > } > > @@ -235,14 +242,12 @@ int do_avb_verify_part(struct cmd_tbl *cmdtp, int > > flag, > > char *cmdline; > > char *extra_args; > > char *slot_suffix = ""; > > + int ret; > > > > bool unlocked = false; > > int res = CMD_RET_FAILURE; > > > > - if (!avb_ops) { > > - printf("AVB 2.0 is not initialized, run 'avb init' first\n"); > > - return CMD_RET_FAILURE; > > - } > > + AVB_OPS_CHECK(avb_ops); > > > > if (argc < 1 || argc > 2) > > return CMD_RET_USAGE; > > @@ -253,9 +258,10 @@ int do_avb_verify_part(struct cmd_tbl *cmdtp, int flag, > > printf("## Android Verified Boot 2.0 version %s\n", > > avb_version_string()); > > > > - if (avb_ops->read_is_device_unlocked(avb_ops, &unlocked) != > > - AVB_IO_RESULT_OK) { > > - printf("Can't determine device lock state.\n"); > > + ret = avb_ops->read_is_device_unlocked(avb_ops, &unlocked); > > + if (ret != AVB_IO_RESULT_OK) { > > + printf("Can't determine device lock state, err = %d\n", > > + ret); > > return CMD_RET_FAILURE; > > } > > > > @@ -302,10 +308,10 @@ int do_avb_verify_part(struct cmd_tbl *cmdtp, int > > flag, > > printf("Corrupted dm-verity metadata detected\n"); > > break; > > case AVB_SLOT_VERIFY_RESULT_ERROR_UNSUPPORTED_VERSION: > > - printf("Unsupported version avbtool was used\n"); > > + printf("Unsupported version of avbtool was used\n"); > > break; > > case AVB_SLOT_VERIFY_RESULT_ERROR_ROLLBACK_INDEX: > > - printf("Checking rollback index failed\n"); > > + printf("Rollback index check failed\n"); > > break; > > case AVB_SLOT_VERIFY_RESULT_ERROR_PUBLIC_KEY_REJECTED: > > printf("Public key was rejected\n"); > > @@ -324,24 +330,23 @@ int do_avb_is_unlocked(struct cmd_tbl *cmdtp, int > > flag, > > int argc, char *const argv[]) > > { > > bool unlock; > > + int ret; > > > > - if (!avb_ops) { > > - printf("AVB not initialized, run 'avb init' first\n"); > > - return CMD_RET_FAILURE; > > - } > > + AVB_OPS_CHECK(avb_ops); > > > > if (argc != 1) { > > printf("--%s(-1)\n", __func__); > > return CMD_RET_USAGE; > > } > > > > - if (avb_ops->read_is_device_unlocked(avb_ops, &unlock) == > > - AVB_IO_RESULT_OK) { > > + ret = avb_ops->read_is_device_unlocked(avb_ops, &unlock); > > + if (ret == AVB_IO_RESULT_OK) { > > printf("Unlocked = %d\n", unlock); > > return CMD_RET_SUCCESS; > > } > > > > - printf("Can't determine device lock state.\n"); > > + printf("Can't determine device lock state, err = %d\n", > > + ret); > > > > return CMD_RET_FAILURE; > > } > > @@ -354,11 +359,9 @@ int do_avb_read_pvalue(struct cmd_tbl *cmdtp, int > > flag, int argc, > > size_t bytes_read; > > void *buffer; > > char *endp; > > + int ret; > > > > - if (!avb_ops) { > > - printf("AVB 2.0 is not initialized, run 'avb init' first\n"); > > - return CMD_RET_FAILURE; > > - } > > + AVB_OPS_CHECK(avb_ops); > > > > if (argc != 3) > > return CMD_RET_USAGE; > > @@ -372,15 +375,16 @@ int do_avb_read_pvalue(struct cmd_tbl *cmdtp, int > > flag, int argc, > > if (!buffer) > > return CMD_RET_FAILURE; > > > > - if (avb_ops->read_persistent_value(avb_ops, name, bytes, buffer, > > - &bytes_read) == AVB_IO_RESULT_OK) { > > + ret = avb_ops->read_persistent_value(avb_ops, name, bytes, > > + buffer, &bytes_read); > > + if (ret == AVB_IO_RESULT_OK) { > > printf("Read %zu bytes, value = %s\n", bytes_read, > > (char *)buffer); > > free(buffer); > > return CMD_RET_SUCCESS; > > } > > > > - printf("Failed to read persistent value\n"); > > + printf("Failed to read persistent value, err = %d\n", ret); > > > > free(buffer); > > > > @@ -392,11 +396,9 @@ int do_avb_write_pvalue(struct cmd_tbl *cmdtp, int > > flag, int argc, > > { > > const char *name; > > const char *value; > > + int ret; > > > > - if (!avb_ops) { > > - printf("AVB 2.0 is not initialized, run 'avb init' first\n"); > > - return CMD_RET_FAILURE; > > - } > > + AVB_OPS_CHECK(avb_ops); > > > > if (argc != 3) > > return CMD_RET_USAGE; > > @@ -404,14 +406,16 @@ int do_avb_write_pvalue(struct cmd_tbl *cmdtp, int > > flag, int argc, > > name = argv[1]; > > value = argv[2]; > > > > - if (avb_ops->write_persistent_value(avb_ops, name, strlen(value) + 1, > > - (const uint8_t *)value) == > > - AVB_IO_RESULT_OK) { > > + ret = avb_ops->write_persistent_value(avb_ops, name, > > + strlen(value) + 1, > > + (const uint8_t *)value); > > + if (ret == AVB_IO_RESULT_OK) { > > printf("Wrote %zu bytes\n", strlen(value) + 1); > > return CMD_RET_SUCCESS; > > } > > > > - printf("Failed to write persistent value\n"); > > + printf("Failed to write persistent value `%s` = `%s`, err = %d\n", > > + name, value, ret); > > > > return CMD_RET_FAILURE; > > } > > -- > > 2.34.1 -- Best regards - Freundliche Grüsse - Meilleures salutations Igor Opaniuk Senior Software Engineer, Embedded & Security E: igor.opan...@foundries.io W: www.foundries.io