From: Mattijs Korpershoek <mkorpershoek@baylibre.com>
To: Igor Opaniuk <igor.opaniuk@foundries.io>, u-boot@lists.denx.de
Cc: Igor Opaniuk <igor.opaniuk@gmail.com>, Tom Rini <trini@konsulko.com>
Subject: Re: [PATCH v2 4/7] cmd: avb: rework prints
Date: Tue, 13 Feb 2024 09:20:52 +0100 [thread overview]
Message-ID: <87eddgg4gr.fsf@baylibre.com> (raw)
In-Reply-To: <20240209192045.3961832-5-igor.opaniuk@foundries.io>
Hi Igor,
Thank you for the patch.
On ven., févr. 09, 2024 at 20:20, Igor Opaniuk <igor.opaniuk@foundries.io> wrote:
> From: Igor Opaniuk <igor.opaniuk@gmail.com>
>
> Simplify and add more context for prints where it's needed.
>
> Signed-off-by: Igor Opaniuk <igor.opaniuk@gmail.com>
Reviewed-by: Mattijs Korpershoek <mkorpershoek@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
next prev parent reply other threads:[~2024-02-13 8:21 UTC|newest]
Thread overview: 17+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-02-09 19:20 [PATCH v2 0/7] AVB: cosmetic adjustments/improvements Igor Opaniuk
2024-02-09 19:20 ` [PATCH v2 1/7] common: avb_verify: don't call mmc_switch_part for SD Igor Opaniuk
2024-02-09 19:37 ` Dragan Simic
2024-02-12 7:05 ` Dan Carpenter
2024-02-12 8:05 ` Igor Opaniuk
2024-02-13 8:13 ` Mattijs Korpershoek
2024-02-13 11:19 ` Igor Opaniuk
2024-02-13 13:31 ` Mattijs Korpershoek
2024-02-09 19:20 ` [PATCH v2 2/7] avb: move SPDX license identifiers to the first line Igor Opaniuk
2024-02-09 19:20 ` [PATCH v2 3/7] common: avb_verify: rework error/debug prints Igor Opaniuk
2024-02-09 19:20 ` [PATCH v2 4/7] cmd: avb: rework prints Igor Opaniuk
2024-02-13 8:20 ` Mattijs Korpershoek [this message]
2024-02-09 19:20 ` [PATCH v2 5/7] common: avb_verify: add str_avb_io_error/str_avb_slot_error Igor Opaniuk
2024-02-09 19:20 ` [PATCH v2 6/7] cmd: avb: rework do_avb_verify_part Igor Opaniuk
2024-02-09 19:20 ` [PATCH v2 7/7] doc: android: avb: sync usage details Igor Opaniuk
2024-02-13 8:22 ` Mattijs Korpershoek
2024-02-13 15:18 ` [PATCH v2 0/7] AVB: cosmetic adjustments/improvements Mattijs Korpershoek
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=87eddgg4gr.fsf@baylibre.com \
--to=mkorpershoek@baylibre.com \
--cc=igor.opaniuk@foundries.io \
--cc=igor.opaniuk@gmail.com \
--cc=trini@konsulko.com \
--cc=u-boot@lists.denx.de \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.