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 v1 4/7] cmd: avb: rework prints
Date: Thu, 08 Feb 2024 15:00:27 +0100 [thread overview]
Message-ID: <875xyzjbt0.fsf@baylibre.com> (raw)
In-Reply-To: <20240206223153.3060433-5-igor.opaniuk@foundries.io>
Hi Igor,
Thank you for the patch.
On mar., févr. 06, 2024 at 23:31, Igor Opaniuk <igor.opaniuk@foundries.io> wrote:
> From: Igor Opaniuk <igor.opaniuk@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.opaniuk@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?
> 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
next prev parent reply other threads:[~2024-02-08 14:00 UTC|newest]
Thread overview: 17+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-02-06 22:31 [PATCH v1 0/7] AVB: cosmetic adjustments/improvements Igor Opaniuk
2024-02-06 22:31 ` [PATCH v1 1/7] common: avb_verify: don't call mmc_switch_part for SD Igor Opaniuk
2024-02-08 13:35 ` Mattijs Korpershoek
2024-02-06 22:31 ` [PATCH v1 2/7] avb: move SPDX license itdentifiers to the first line Igor Opaniuk
2024-02-08 13:41 ` Mattijs Korpershoek
2024-02-06 22:31 ` [PATCH v1 3/7] common: avb_verify: rework error/debug prints Igor Opaniuk
2024-02-08 13:43 ` Mattijs Korpershoek
2024-02-06 22:31 ` [PATCH v1 4/7] cmd: avb: rework prints Igor Opaniuk
2024-02-08 14:00 ` Mattijs Korpershoek [this message]
2024-02-09 9:56 ` Igor Opaniuk
2024-02-06 22:31 ` [PATCH v1 5/7] common: avb_verify: add str_avb_io_error/str_avb_slot_error Igor Opaniuk
2024-02-08 14:03 ` Mattijs Korpershoek
2024-02-06 22:31 ` [PATCH v1 6/7] cmd: avb: rework do_avb_verify_part Igor Opaniuk
2024-02-09 9:17 ` Mattijs Korpershoek
2024-02-06 22:31 ` [PATCH v1 7/7] doc: android: avb: add slot_suffix param details Igor Opaniuk
2024-02-08 14:12 ` Mattijs Korpershoek
2024-02-09 9:50 ` Igor Opaniuk
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=875xyzjbt0.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