[PATCH] Patch to handle partitions with alternate names.

[Ole Kristian Lona]

Thanks.


> Hi Ole Kristian,
> 
> This patch has some formal issues. The patch title should start with
> "tplink-safeloader:";

Sorry, bad mistake, hopefully won't ever happen again!

> see `git log src/tplink-safeloader.c` for examples. Furthermore, there
> are trailing white
> spaces, you've used DOS line endings (CRLF), and there's a number of
> other formatting
> issues. You can use openwrt.git:scripts/checkpatch.pl to detect these
> issues before
> submitting patches.

Are you sure about this CRLF? I do all my development work in VSCode for 
Linux, the file is clearly marked as LF in VSCode, and cat -vTE doesn't 
show any ^Ms.

Thanks for the checkoath.pl part. I didn't know about it, but will be 
using it from now on.
> 
> On Fri, 2022-04-29 at 08:40 +0200, oklo at oklona.net wrote:
>> Some devices, specifically Deco M4R-v3 / M5. These devices have
>> fallback partitions which will be used in case the device
>> determines that the primary partition set is unbootable.
>> 
>> It is believed that the backup partitions are populated by some
>> mechanism in the OEM firmware, meaning a fallback to
>> backup-partitions will effectively revert the device back to OEM 
>> software.
> 
> How these back-up partitions are created is not very relevant for this
> patch, so you can
> leave out the second paragraph. What is important to note however, is 
> that these
> partitions do not use the standard list of partition names we've
> assumed until now. (The
> 'why is this patch needed' part of the commit message)
> 

Ok, noted.

>> 
>> Signed-off-by: Ole Kristian Lona <oklo at oklona.net>
>> ---
>>  src/tplink-safeloader.c | 67 
>> +++++++++++++++++++++++++++++++++--------
>>  1 file changed, 54 insertions(+), 13 deletions(-)
>> 
>> diff --git a/src/tplink-safeloader.c b/src/tplink-safeloader.c
>> index fc46124..ccb384f 100644
>> --- a/src/tplink-safeloader.c
>> +++ b/src/tplink-safeloader.c
>> @@ -53,6 +53,15 @@ struct flash_partition_entry {
>>         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;
>> +};
> 
> trailing whitespace
> 
>> +
>>  /** 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}
>> @@ -2982,8 +2992,8 @@ 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);
>>  
>>         char *s = (char *)entry.data, *end = (char *)(s+entry.size);
>>  
>> @@ -3018,14 +3028,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,
>>                         info->soft_ver.text, len, info->part_trail);
>>         }
>>  
>> @@ -3055,11 +3065,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);
>>  }
>>  
>> @@ -3298,7 +3308,7 @@ static void build_image(const char *output,
>>                 bool sysupgrade,
>>                 struct device_info *info) {
>>  
>> -       size_t i;
>> +       size_t i;
> 
> spurious change due to added whitespace
> 
>>  
>>         struct image_partition_entry parts[7] = {};
>>  
>> @@ -3306,6 +3316,8 @@ static void build_image(const char *output,
>>         struct flash_partition_entry *os_image_partition = NULL;
>>         struct flash_partition_entry *file_system_partition = NULL;
>>         size_t firmware_partition_index = 0;
>> +       char fs_name[32];
>> +       char os_name[32];
> 
> These extra variables aren't required. You can just pass the const
> char * from the
> partition name table to the relevant functions. The device_info struct
> that is passed into
> this function has a longer lifetime than (any data generated by) this 
> function.

Well, it depends on your preference on warnings. By not using these, you 
change pointers that are
interpreted as const, and get warnings about that, which is the only 
reason I added these.
I normally try as hard as I can to create code that does not create 
spurious warnings.

> 
>>  
>>         for (i = 0; info->partitions[i].name; i++) {
>>                 if (!strcmp(info->partitions[i].name, "firmware"))
>> @@ -3330,7 +3342,13 @@ 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";
>> +               if(info->partition_names.file_system==NULL)
>> +                       file_system_partition->name = "file-system";
>> +               else{
>> +                       strcpy(fs_name, 
>> info->partition_names.file_system);
>> +                       file_system_partition->name = fs_name;
>> +               }       
>> +               
> 
> formatting issues: no spaces around '==', 'else{', trailing whitespace
> 
> This can be simplified into:
> 
> if (info->partition_names.file_system)
> 	file_system_partition->name = info->partition_names.file_system;
> else
> 	file_system_partition->name = "file-system";
> 

I was very much in doubt about this. Mostly, I wrote things in the 
simple way, but seem to have forgotten about this one.
Most C programmers recommend using curly braces for readability, 
although I clearly prefer the simple notation.

> 
> Same comments for the rest of the patch.
> 
> Best,
> Sander

I will send a new version of the path, with all of this fixed tomorrow. 
-But I am unsure on how to proceed with the fs_name[32]
and os_name[32] variables, since removing them will make the code 
generate warnings.


Thanks,

Ole Kristian