[PATCH v1 4/7] cmd: avb: rework prints
Mattijs Korpershoek
mkorpershoek at baylibre.com
Thu Feb 8 15:00:27 CET 2024
Hi Igor,
Thank you for the patch.
On mar., févr. 06, 2024 at 23:31, Igor Opaniuk <igor.opaniuk at foundries.io> wrote:
> From: Igor Opaniuk <igor.opaniuk at 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 at 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
More information about the U-Boot
mailing list