[PATCH V3] tplink-safeloader: Patch to handle partitions with alternate names

[Sander Vanheule]

Hi Ole Kristian,

Could you CC Rafał and me (and other reviewers) on revised patches? At least we should get
the patch without DMARC wrapping in that case.

On Thu, 2022-05-05 at 17:41 +0200, Ole Kristian Lona wrote:
>     Some devices, specifically Deco M4R-v3 / M5 have partition names that
>     deviate from the scheme other devices use. They have an added "@0"
>     and "@1" for some patition names. The devices have
>     fallback partitions which will be used in case the device
>     determines that the primary partition set is unbootable.
> 
>     This patch introduces an option to set these alternate partition
>     names in the device definition of tplink-safeloader.
> 
>     Signed-off-by: Ole Kristian Lona <oklo at oklona.net>
> ---
>  src/tplink-safeloader.c | 58 ++++++++++++++++++++++++++++++++---------
>  1 file changed, 45 insertions(+), 13 deletions(-)
> 
> diff --git a/src/tplink-safeloader.c b/src/tplink-safeloader.c
> index 1c08a33..2adcaf6 100644
> --- a/src/tplink-safeloader.c
> +++ b/src/tplink-safeloader.c
> @@ -48,11 +48,20 @@ struct image_partition_entry {
>  
>  /** A flash partition table entry */
>  struct flash_partition_entry {
> -       char *name;
> +       const char *name;
>         uint32_t base;
>         uint32_t size;
>  };
>  
> +/** Flash partition names table entry */
> +struct factory_partition_names {
> +       const char *partition_table;
> +       const char *soft_ver;
> +       const char *os_image;
> +       const char *file_system;
> +       const char *extra_para;
> +};
> +
>  /** Partition trailing padding definitions
>   * Values 0x00 to 0xff are reserved to indicate the padding value
>   * Values from 0x100 are reserved to indicate other behaviour */
> @@ -89,6 +98,7 @@ struct device_info {
>         struct flash_partition_entry partitions[MAX_PARTITIONS+1];
>         const char *first_sysupgrade_partition;
>         const char *last_sysupgrade_partition;
> +       struct factory_partition_names partition_names;
>  };
>  
>  #define SOFT_VER_TEXT(_t) {.type = SOFT_VER_TYPE_TEXT, .text = _t}
> @@ -2962,6 +2972,21 @@ static struct image_partition_entry alloc_image_partition(const
> char *name, size
>         return entry;
>  }
>  
> +/** Sets up default partition names whenever custom names aren't specified */
> +static void set_partition_names(struct device_info *info)
> +{
> +       if (!info->partition_names.partition_table)
> +               info->partition_names.partition_table = "partition-table";
> +       if (!info->partition_names.soft_ver)
> +               info->partition_names.soft_ver = "soft-version";
> +       if (!info->partition_names.os_image)
> +               info->partition_names.os_image = "os-image";
> +       if (!info->partition_names.file_system)
> +               info->partition_names.file_system = "file-system";
> +       if (!info->partition_names.extra_para)
> +               info->partition_names.extra_para = "extra-para";
> +}
> +
>  /** Frees an image partition */
>  static void free_image_partition(struct image_partition_entry entry) {
>         free(entry.data);
> @@ -2982,8 +3007,10 @@ static void set_source_date_epoch() {
>  }
>  
>  /** Generates the partition-table partition */
> -static struct image_partition_entry make_partition_table(const struct
> flash_partition_entry *p) {
> -       struct image_partition_entry entry = alloc_image_partition("partition-table",
> 0x800);
> +static struct image_partition_entry make_partition_table(const char *name,
> +       const struct flash_partition_entry *p)
> +{
> +       struct image_partition_entry entry = alloc_image_partition(name, 0x800);

If you pass a device_info reference instead of the flash_partition_entry list, you can
draw all the required information from that struct and don't need to pass an additional
string. make_soft_version(), make_support_list(), and make_extra_para() already take a
struct device_info * argument.

>  
>         char *s = (char *)entry.data, *end = (char *)(s+entry.size);
>  
> @@ -3018,14 +3045,14 @@ static inline uint8_t bcd(uint8_t v) {
>  
>  
>  /** Generates the soft-version partition */
> -static struct image_partition_entry make_soft_version(
> +static struct image_partition_entry make_soft_version(const char *name,
>         const struct device_info *info, uint32_t rev)
>  {
>         /** If an info string is provided, use this instead of
>          * the structured data, and include the null-termination */
>         if (info->soft_ver.type == SOFT_VER_TYPE_TEXT) {
>                 uint32_t len = strlen(info->soft_ver.text) + 1;
> -               return init_meta_partition_entry("soft-version",
> +               return init_meta_partition_entry(name,

You should be able to draw the partition name from device_info, which you've updated to
contain the required value(s).

>                         info->soft_ver.text, len, info->part_trail);
>         }
>  
> @@ -3055,11 +3082,11 @@ static struct image_partition_entry make_soft_version(
>         };
>  
>         if (info->soft_ver_compat_level == 0)
> -               return init_meta_partition_entry("soft-version", &s,
> +               return init_meta_partition_entry(name, &s,
>                         (uint8_t *)(&s.compat_level) - (uint8_t *)(&s),
>                         info->part_trail);
>         else
> -               return init_meta_partition_entry("soft-version", &s,
> +               return init_meta_partition_entry(name, &s,
>                         sizeof(s), info->part_trail);
>  }
>  

Shouldn't you also modify make_support_list() and make_extra_para()?

> @@ -3307,6 +3334,8 @@ static void build_image(const char *output,
>         struct flash_partition_entry *file_system_partition = NULL;
>         size_t firmware_partition_index = 0;
>  
> +       set_partition_names(info);
> +
>         for (i = 0; info->partitions[i].name; i++) {
>                 if (!strcmp(info->partitions[i].name, "firmware"))
>                 {
> @@ -3330,7 +3359,8 @@ static void build_image(const char *output,
>                 for (i = MAX_PARTITIONS-1; i >= firmware_partition_index + 1; i--)
>                         info->partitions[i+1] = info->partitions[i];
>  
> -               file_system_partition->name = "file-system";
> +               file_system_partition->name = info->partition_names.file_system;
> +

You can keep these two lines together, no need to add an empty line.

>                 file_system_partition->base = firmware_partition->base + kernel.st_size;
>  
>                 /* Align partition start to erase blocks for factory images only */
> @@ -3339,15 +3369,17 @@ static void build_image(const char *output,
>  
>                 file_system_partition->size = firmware_partition->size -
> file_system_partition->base;
>  
> -               os_image_partition->name = "os-image";
> +               os_image_partition->name = info->partition_names.os_image;
> +

Same here.

>                 os_image_partition->size = kernel.st_size;
>         }
>  
> -       parts[0] = make_partition_table(info->partitions);
> -       parts[1] = make_soft_version(info, rev);
> +       parts[0] = make_partition_table(info->partition_names.partition_table, info-
> >partitions);
> +       parts[1] = make_soft_version(info->partition_names.soft_ver, info, rev);
>         parts[2] = make_support_list(info);
> -       parts[3] = read_file("os-image", kernel_image, false, NULL);
> -       parts[4] = read_file("file-system", rootfs_image, add_jffs2_eof,
> file_system_partition);
> +       parts[3] = read_file(info->partition_names.os_image, kernel_image, false, NULL);
> +       parts[4] = read_file(info->partition_names.file_system, rootfs_image,
> add_jffs2_eof, file_system_partition);
> +

No need to add a (second) empty line here

>  
>         /* Some devices need the extra-para partition to accept the firmware */
>         if (strcasecmp(info->id, "ARCHER-A6-V3") == 0 ||

Best,
Sander