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

[Ole Kristian Lona]

Thank you, Sander!


Next version will be posted in a few hours, unless one of you strongly disagrees with my conclusions regarding "support-list" as outlined below.

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

Sure, will do! I did replace the -all with ~all in my SPF record, but apparently, that obviously wasn't enough. 

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

Good call! Fixed in my next version of the patch!

>>  
>>         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).
>
Done! Ready in next version of the patch.

>>                         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()?

I believe "support-list" will never change the name, and it definitely hasn't in the firmwares I have seen. It sounds more logical to me that they always keep that name, so any device can check the support list for any firmware.

Extra-para is only used for a very few devices, but I now added an option for using a different name for that as well.

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