Hi Igor,
Thank you for the patch.
On ven., févr. 09, 2024 at 20:20, Igor Opaniuk <igor.opan...@foundries.io>
wrote:
> From: Igor Opaniuk <igor.opan...@gmail.com>
>
> Simplify and add more context for prints where it's needed.
>
> Signed-off-by: Igor Opaniuk <igor.opan...@gmail.com>
Reviewed-by: Mattijs Korpershoek <mkorpersh...@baylibre.com>
> ---
>
> Changes in v2:
> - Drop AVB_OPS_CHECK macro and leave previous check
>
> cmd/avb.c | 123 +++++++++++++++++++++++++++++++++---------------------
> 1 file changed, 75 insertions(+), 48 deletions(-)
>
> diff --git a/cmd/avb.c b/cmd/avb.c
> index ce8b63873f2..62a3ee18e7f 100644
> --- a/cmd/avb.c
> +++ b/cmd/avb.c
> @@ -11,6 +11,7 @@
> #include <mmc.h>
>
> #define AVB_BOOTARGS "avb_bootargs"
> +
> static struct AvbOps *avb_ops;
>
> int do_avb_init(struct cmd_tbl *cmdtp, int flag, int argc, char *const
> argv[])
> @@ -28,8 +29,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,10 +44,11 @@ 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;
> + printf("AVB is not initialized, please run 'avb init <id>'\n");
> + return CMD_RET_FAILURE;
> }
>
> if (argc != 5)
> @@ -55,14 +59,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,10 +79,11 @@ 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;
> + printf("AVB is not initialized, please run 'avb init <id>'\n");
> + return CMD_RET_FAILURE;
> }
>
> if (argc != 4)
> @@ -94,8 +100,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 +114,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,9 +128,10 @@ 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");
> + printf("AVB is not initialized, please run 'avb init <id>'\n");
> return CMD_RET_FAILURE;
> }
>
> @@ -134,13 +143,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,9 +161,10 @@ 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");
> + printf("AVB is not initialized, please run 'avb init <id>'\n");
> return CMD_RET_FAILURE;
> }
>
> @@ -161,13 +173,14 @@ int do_avb_read_rb(struct cmd_tbl *cmdtp, int flag, int
> argc,
>
> 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,9 +190,10 @@ 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");
> + printf("AVB is not initialized, please run 'avb init <id>'\n");
> return CMD_RET_FAILURE;
> }
>
> @@ -189,11 +203,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,9 +218,10 @@ 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");
> + printf("AVB is not initialized, please run 'avb init <id>'\n");
> return CMD_RET_FAILURE;
> }
>
> @@ -214,14 +230,16 @@ int do_avb_get_uuid(struct cmd_tbl *cmdtp, int flag,
>
> 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,12 +253,13 @@ 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");
> + printf("AVB is not initialized, please run 'avb init <id>'\n");
> return CMD_RET_FAILURE;
> }
>
> @@ -253,9 +272,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 +322,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,9 +344,10 @@ 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");
> + printf("AVB is not initialized, please run 'avb init <id>'\n");
> return CMD_RET_FAILURE;
> }
>
> @@ -335,13 +356,14 @@ int do_avb_is_unlocked(struct cmd_tbl *cmdtp, int flag,
> 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,9 +376,10 @@ 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");
> + printf("AVB is not initialized, please run 'avb init <id>'\n");
> return CMD_RET_FAILURE;
> }
>
> @@ -372,15 +395,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,9 +416,10 @@ 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");
> + printf("AVB is not initialized, please run 'avb init <id>'\n");
> return CMD_RET_FAILURE;
> }
>
> @@ -404,14 +429,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
