[PATCH v1 4/7] cmd: avb: rework prints

Igor Opaniuk igor.opaniuk at foundries.io
Fri Feb 9 10:56:22 CET 2024


Hi Mattijs,

On Thu, Feb 8, 2024 at 3:00 PM Mattijs Korpershoek
<mkorpershoek at baylibre.com> wrote:
>
> 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?
Yeah, I was just trying to find a tradeoff between readability/code duplication
and coding style.

Will drop it in v2. Thanks!

>
> >  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



-- 
Best regards - Freundliche Grüsse - Meilleures salutations

Igor Opaniuk
Senior Software Engineer, Embedded & Security
E: igor.opaniuk at foundries.io
W: www.foundries.io


More information about the U-Boot mailing list