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 a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox