diff mbox series

[2/2] android_ab: Fix ANDROID_AB_BACKUP_OFFSET

Message ID 20240307161711.3177729-3-colin.mcallister@garmin.com
State Superseded
Delegated to: Mattijs Korpershoek
Headers show
Series Fix Android A/B backup | expand

Commit Message

McAllister, Colin March 7, 2024, 4:17 p.m. UTC 
Currently, setting CONFIG_AB_BACKUP_OFFSET in a target's defconfig will
not actually enable the #if protected code in android_ab.c. This is
because "CONFIG_" should have been prepended to the config macro, or the
macros defined in kconfig.h could have been used.

Each use of AB_BACKUP_OFFSET is now wrapped within CONFIG_VAL().

Signed-off-by: Colin McAllister <colin.mcallister@garmin.com>
Cc: Joshua Watt <JPEWhacker@gmail.com>
Cc: Simon Glass <sjg@chromium.org>
---
 boot/android_ab.c | 22 +++++++++++-----------
 1 file changed, 11 insertions(+), 11 deletions(-)

--
2.43.2

Comments

Igor Opaniuk March 7, 2024, 5:50 p.m. UTC | #1 
Hello Colin,

+CC Mattijs, Sam

On Thu, Mar 7, 2024 at 5:19 PM Colin McAllister <colin.mcallister@garmin.com>
wrote:

> Currently, setting CONFIG_AB_BACKUP_OFFSET in a target's defconfig will
> not actually enable the #if protected code in android_ab.c. This is
> because "CONFIG_" should have been prepended to the config macro, or the
> macros defined in kconfig.h could have been used.
>
> Each use of AB_BACKUP_OFFSET is now wrapped within CONFIG_VAL().
>
> Signed-off-by: Colin McAllister <colin.mcallister@garmin.com>
> Cc: Joshua Watt <JPEWhacker@gmail.com>
> Cc: Simon Glass <sjg@chromium.org>
> ---
>  boot/android_ab.c | 22 +++++++++++-----------
>  1 file changed, 11 insertions(+), 11 deletions(-)
>
> diff --git a/boot/android_ab.c b/boot/android_ab.c
> index 9a3d15ec60..a80040749d 100644
> --- a/boot/android_ab.c
> +++ b/boot/android_ab.c
> @@ -191,7 +191,7 @@ int ab_select_slot(struct blk_desc *dev_desc, struct
> disk_partition *part_info,
>         int slot, i, ret;
>         bool store_needed = false;
>         char slot_suffix[4];
> -#if ANDROID_AB_BACKUP_OFFSET
> +#if CONFIG_VAL(ANDROID_AB_BACKUP_OFFSET)
>         struct bootloader_control *backup_abc = NULL;
>  #endif
>
> @@ -205,9 +205,9 @@ int ab_select_slot(struct blk_desc *dev_desc, struct
> disk_partition *part_info,
>                 return ret;
>         }
>
> -#if ANDROID_AB_BACKUP_OFFSET
> +#if CONFIG_VAL(ANDROID_AB_BACKUP_OFFSET)
>         ret = ab_control_create_from_disk(dev_desc, part_info, &backup_abc,
> -                                         ANDROID_AB_BACKUP_OFFSET);
> +
>  CONFIG_VAL(ANDROID_AB_BACKUP_OFFSET));
>         if (ret < 0) {
>                 free(abc);
>                 return ret;
> @@ -218,7 +218,7 @@ int ab_select_slot(struct blk_desc *dev_desc, struct
> disk_partition *part_info,
>         if (abc->crc32_le != crc32_le) {
>                 log_err("ANDROID: Invalid CRC-32 (expected %.8x, found
> %.8x),",
>                         crc32_le, abc->crc32_le);
> -#if ANDROID_AB_BACKUP_OFFSET
> +#if CONFIG_VAL(ANDROID_AB_BACKUP_OFFSET)
>                 crc32_le = ab_control_compute_crc(backup_abc);
>                 if (backup_abc->crc32_le != crc32_le) {
>                         log_err("ANDROID: Invalid backup CRC-32 ");
> @@ -230,13 +230,13 @@ int ab_select_slot(struct blk_desc *dev_desc, struct
> disk_partition *part_info,
>
>                         ret = ab_control_default(abc);
>                         if (ret < 0) {
> -#if ANDROID_AB_BACKUP_OFFSET
> +#if CONFIG_VAL(ANDROID_AB_BACKUP_OFFSET)
>                                 free(backup_abc);
>  #endif
>                                 free(abc);
>                                 return -ENODATA;
>                         }
> -#if ANDROID_AB_BACKUP_OFFSET
> +#if CONFIG_VAL(ANDROID_AB_BACKUP_OFFSET)
>                 } else {
>                         /*
>                          * Backup is valid. Copy it to the primary
> @@ -249,7 +249,7 @@ int ab_select_slot(struct blk_desc *dev_desc, struct
> disk_partition *part_info,
>
>         if (abc->magic != BOOT_CTRL_MAGIC) {
>                 log_err("ANDROID: Unknown A/B metadata: %.8x\n",
> abc->magic);
> -#if ANDROID_AB_BACKUP_OFFSET
> +#if CONFIG_VAL(ANDROID_AB_BACKUP_OFFSET)
>                 free(backup_abc);
>  #endif
>                 free(abc);
> @@ -259,7 +259,7 @@ int ab_select_slot(struct blk_desc *dev_desc, struct
> disk_partition *part_info,
>         if (abc->version > BOOT_CTRL_VERSION) {
>                 log_err("ANDROID: Unsupported A/B metadata version:
> %.8x\n",
>                         abc->version);
> -#if ANDROID_AB_BACKUP_OFFSET
> +#if CONFIG_VAL(ANDROID_AB_BACKUP_OFFSET)
>                 free(backup_abc);
>  #endif
>                 free(abc);
> @@ -338,7 +338,7 @@ int ab_select_slot(struct blk_desc *dev_desc, struct
> disk_partition *part_info,
>                 abc->crc32_le = ab_control_compute_crc(abc);
>                 ret = ab_control_store(dev_desc, part_info, abc, 0);
>                 if (ret < 0) {
> -#if ANDROID_AB_BACKUP_OFFSET
> +#if CONFIG_VAL(ANDROID_AB_BACKUP_OFFSET)
>                         free(backup_abc);
>  #endif
>                         free(abc);
> @@ -346,14 +346,14 @@ int ab_select_slot(struct blk_desc *dev_desc, struct
> disk_partition *part_info,
>                 }
>         }
>
> -#if ANDROID_AB_BACKUP_OFFSET
> +#if CONFIG_VAL(ANDROID_AB_BACKUP_OFFSET)
>         /*
>          * If the backup doesn't match the primary, write the primary
>          * to the backup offset
>          */
>         if (memcmp(backup_abc, abc, sizeof(*abc)) != 0) {
>                 ret = ab_control_store(dev_desc, part_info, abc,
> -                                      ANDROID_AB_BACKUP_OFFSET);
> +
> CONFIG_VAL(ANDROID_AB_BACKUP_OFFSET));
>                 if (ret < 0) {
>                         free(backup_abc);
>                         free(abc);
>

Thanks for the patch. The only suggestion: I see no reason in wrapping it in
preprocessor conditionals (in this particular case), better to use C
conditionals instead (with CONFIG_VAL macro) as explained in [1].

-
> 2.43.2
>
>
> ________________________________
>
> CONFIDENTIALITY NOTICE: This email and any attachments are for the sole
> use of the intended recipient(s) and contain information that may be Garmin
> confidential and/or Garmin legally privileged. If you have received this
> email in error, please notify the sender by reply email and delete the
> message. Any disclosure, copying, distribution or use of this communication
> (including attachments) by someone other than the intended recipient is
> prohibited. Thank you.
>

[1]
https://www.kernel.org/doc/html/latest/process/coding-style.html#conditional-compilation
diff mbox series

Patch

diff --git a/boot/android_ab.c b/boot/android_ab.c
index 9a3d15ec60..a80040749d 100644
--- a/boot/android_ab.c
+++ b/boot/android_ab.c
@@ -191,7 +191,7 @@  int ab_select_slot(struct blk_desc *dev_desc, struct disk_partition *part_info,
        int slot, i, ret;
        bool store_needed = false;
        char slot_suffix[4];
-#if ANDROID_AB_BACKUP_OFFSET
+#if CONFIG_VAL(ANDROID_AB_BACKUP_OFFSET)
        struct bootloader_control *backup_abc = NULL;
 #endif

@@ -205,9 +205,9 @@  int ab_select_slot(struct blk_desc *dev_desc, struct disk_partition *part_info,
                return ret;
        }

-#if ANDROID_AB_BACKUP_OFFSET
+#if CONFIG_VAL(ANDROID_AB_BACKUP_OFFSET)
        ret = ab_control_create_from_disk(dev_desc, part_info, &backup_abc,
-                                         ANDROID_AB_BACKUP_OFFSET);
+                                         CONFIG_VAL(ANDROID_AB_BACKUP_OFFSET));
        if (ret < 0) {
                free(abc);
                return ret;
@@ -218,7 +218,7 @@  int ab_select_slot(struct blk_desc *dev_desc, struct disk_partition *part_info,
        if (abc->crc32_le != crc32_le) {
                log_err("ANDROID: Invalid CRC-32 (expected %.8x, found %.8x),",
                        crc32_le, abc->crc32_le);
-#if ANDROID_AB_BACKUP_OFFSET
+#if CONFIG_VAL(ANDROID_AB_BACKUP_OFFSET)
                crc32_le = ab_control_compute_crc(backup_abc);
                if (backup_abc->crc32_le != crc32_le) {
                        log_err("ANDROID: Invalid backup CRC-32 ");
@@ -230,13 +230,13 @@  int ab_select_slot(struct blk_desc *dev_desc, struct disk_partition *part_info,

                        ret = ab_control_default(abc);
                        if (ret < 0) {
-#if ANDROID_AB_BACKUP_OFFSET
+#if CONFIG_VAL(ANDROID_AB_BACKUP_OFFSET)
                                free(backup_abc);
 #endif
                                free(abc);
                                return -ENODATA;
                        }
-#if ANDROID_AB_BACKUP_OFFSET
+#if CONFIG_VAL(ANDROID_AB_BACKUP_OFFSET)
                } else {
                        /*
                         * Backup is valid. Copy it to the primary
@@ -249,7 +249,7 @@  int ab_select_slot(struct blk_desc *dev_desc, struct disk_partition *part_info,

        if (abc->magic != BOOT_CTRL_MAGIC) {
                log_err("ANDROID: Unknown A/B metadata: %.8x\n", abc->magic);
-#if ANDROID_AB_BACKUP_OFFSET
+#if CONFIG_VAL(ANDROID_AB_BACKUP_OFFSET)
                free(backup_abc);
 #endif
                free(abc);
@@ -259,7 +259,7 @@  int ab_select_slot(struct blk_desc *dev_desc, struct disk_partition *part_info,
        if (abc->version > BOOT_CTRL_VERSION) {
                log_err("ANDROID: Unsupported A/B metadata version: %.8x\n",
                        abc->version);
-#if ANDROID_AB_BACKUP_OFFSET
+#if CONFIG_VAL(ANDROID_AB_BACKUP_OFFSET)
                free(backup_abc);
 #endif
                free(abc);
@@ -338,7 +338,7 @@  int ab_select_slot(struct blk_desc *dev_desc, struct disk_partition *part_info,
                abc->crc32_le = ab_control_compute_crc(abc);
                ret = ab_control_store(dev_desc, part_info, abc, 0);
                if (ret < 0) {
-#if ANDROID_AB_BACKUP_OFFSET
+#if CONFIG_VAL(ANDROID_AB_BACKUP_OFFSET)
                        free(backup_abc);
 #endif
                        free(abc);
@@ -346,14 +346,14 @@  int ab_select_slot(struct blk_desc *dev_desc, struct disk_partition *part_info,
                }
        }

-#if ANDROID_AB_BACKUP_OFFSET
+#if CONFIG_VAL(ANDROID_AB_BACKUP_OFFSET)
        /*
         * If the backup doesn't match the primary, write the primary
         * to the backup offset
         */
        if (memcmp(backup_abc, abc, sizeof(*abc)) != 0) {
                ret = ab_control_store(dev_desc, part_info, abc,
-                                      ANDROID_AB_BACKUP_OFFSET);
+                                      CONFIG_VAL(ANDROID_AB_BACKUP_OFFSET));
                if (ret < 0) {
                        free(backup_abc);
                        free(abc);