diff mbox series

tplink-safeloader: Patch to handle partitions with alternate names.

Message ID mailman.11211.1651508692.441203.openwrt-devel@lists.openwrt.org
State Changes Requested
Delegated to: Sander Vanheule
Headers show
Series tplink-safeloader: Patch to handle partitions with alternate names. | expand

Commit Message

Ole Kristian Lona via openwrt-devel May 2, 2022, 4:24 p.m. UTC
The sender domain has a DMARC Reject/Quarantine policy which disallows
sending mailing list messages using the original "From" header.

To mitigate this problem, the original message has been wrapped
automatically by the mailing list software.
From: Ole Kristian Lona <oklo@oklona.net>

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.

Signed-off-by: Ole Kristian Lona <oklo@oklona.net>
---
 src/tplink-safeloader.c | 67 +++++++++++++++++++++++++++++++++--------
 1 file changed, 55 insertions(+), 12 deletions(-)

Comments

Rafał Miłecki May 3, 2022, 7:09 a.m. UTC | #1
On 2.05.2022 18:24, Ole Kristian Lona via openwrt-devel wrote:
> The sender domain has a DMARC Reject/Quarantine policy which disallows
> sending mailing list messages using the original "From" header.
> 
> To mitigate this problem, the original message has been wrapped
> automatically by the mailing list software.

Please kindly fix your domain setup. That ML workaround with forwarding
makes it hard to read / review / apply your changes.

 > @@ -3306,6 +3318,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];
 >
 >  	for (i = 0; info->partitions[i].name; i++) {
 >  		if (!strcmp(info->partitions[i].name, "firmware"))

I don't think you need those fs_name / os_name. Just add "const" where
it's needed.


 > @@ -3339,15 +3359,38 @@  static void build_image(const char *output,
 >
 >  		file_system_partition->size = firmware_partition->size - file_system_partition->base;
 >
 > -		os_image_partition->name = "os-image";
 > +		if (info->partition_names.os_image == NULL)
 > +			os_image_partition->name = "os-image";
 > +		else {
 > +			strcpy(os_name, info->partition_names.os_image);
 > +			os_image_partition->name = os_name;
 > +		}
 > +
 >  		os_image_partition->size = kernel.st_size;
 >  	}
 >
 > -	parts[0] = make_partition_table(info->partitions);
 > -	parts[1] = make_soft_version(info, rev);
 > +	if (info->partition_names.partition_table == NULL)
 > +		parts[0] = make_partition_table("partition-table", info->partitions);
 > +	else
 > +		parts[0] = make_partition_table(info->partition_names.partition_table, info->partitions);
 > +
 > +	if (info->partition_names.soft_ver == NULL)
 > +		parts[1] = make_soft_version("soft-version", info, rev);
 > +	else
 > +		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);
 > +
 > +	if (info->partition_names.os_image == NULL)
 > +		parts[3] = read_file("os-image", kernel_image, false, NULL);
 > +	else
 > +		parts[3] = read_file(info->partition_names.os_image, kernel_image, false, NULL);
 > +
 > +	if (info->partition_names.file_system == NULL)
 > +		parts[4] = read_file("file-system", rootfs_image, add_jffs2_eof, file_system_partition);
 > +	else
 > +		parts[4] = read_file(info->partition_names.file_system, rootfs_image, add_jffs2_eof, file_system_partition);
 > +
 >
 >  	/* Some devices need the extra-para partition to accept the firmware */
 >  	if (strcasecmp(info->id, "ARCHER-A6-V3") == 0 ||

We now have all those (foo == NULL) checks everywhere. We also have
default names inlined all around.

What about adding new function that will setup default names if custom
ones are not specified? That would gather all checking code in one place
and would let you move all NULL checks to a single place.

set_default_names()
{
	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";
	(...)
}
Ole Kristian Lona May 5, 2022, 7:15 a.m. UTC | #2
The sender domain has a DMARC Reject/Quarantine policy which disallows
sending mailing list messages using the original "From" header.

To mitigate this problem, the original message has been wrapped
automatically by the mailing list software.
Thank you! Very good suggestion, I added this to my newest version, and sent it in now.

Have tried to fix SPF, so hope the mail looks better now.


Best regards,

Ole Kristian Lona

On 03/05/2022, 09:09, "Rafał Miłecki" <zajec5@gmail.com> wrote:

    On 2.05.2022 18:24, Ole Kristian Lona via openwrt-devel wrote:
    > The sender domain has a DMARC Reject/Quarantine policy which disallows
    > sending mailing list messages using the original "From" header.
    > 
    > To mitigate this problem, the original message has been wrapped
    > automatically by the mailing list software.

    Please kindly fix your domain setup. That ML workaround with forwarding
    makes it hard to read / review / apply your changes.

     > @@ -3306,6 +3318,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];
     >
     >  	for (i = 0; info->partitions[i].name; i++) {
     >  		if (!strcmp(info->partitions[i].name, "firmware"))

    I don't think you need those fs_name / os_name. Just add "const" where
    it's needed.


     > @@ -3339,15 +3359,38 @@  static void build_image(const char *output,
     >
     >  		file_system_partition->size = firmware_partition->size - file_system_partition->base;
     >
     > -		os_image_partition->name = "os-image";
     > +		if (info->partition_names.os_image == NULL)
     > +			os_image_partition->name = "os-image";
     > +		else {
     > +			strcpy(os_name, info->partition_names.os_image);
     > +			os_image_partition->name = os_name;
     > +		}
     > +
     >  		os_image_partition->size = kernel.st_size;
     >  	}
     >
     > -	parts[0] = make_partition_table(info->partitions);
     > -	parts[1] = make_soft_version(info, rev);
     > +	if (info->partition_names.partition_table == NULL)
     > +		parts[0] = make_partition_table("partition-table", info->partitions);
     > +	else
     > +		parts[0] = make_partition_table(info->partition_names.partition_table, info->partitions);
     > +
     > +	if (info->partition_names.soft_ver == NULL)
     > +		parts[1] = make_soft_version("soft-version", info, rev);
     > +	else
     > +		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);
     > +
     > +	if (info->partition_names.os_image == NULL)
     > +		parts[3] = read_file("os-image", kernel_image, false, NULL);
     > +	else
     > +		parts[3] = read_file(info->partition_names.os_image, kernel_image, false, NULL);
     > +
     > +	if (info->partition_names.file_system == NULL)
     > +		parts[4] = read_file("file-system", rootfs_image, add_jffs2_eof, file_system_partition);
     > +	else
     > +		parts[4] = read_file(info->partition_names.file_system, rootfs_image, add_jffs2_eof, file_system_partition);
     > +
     >
     >  	/* Some devices need the extra-para partition to accept the firmware */
     >  	if (strcasecmp(info->id, "ARCHER-A6-V3") == 0 ||

    We now have all those (foo == NULL) checks everywhere. We also have
    default names inlined all around.

    What about adding new function that will setup default names if custom
    ones are not specified? That would gather all checking code in one place
    and would let you move all NULL checks to a single place.

    set_default_names()
    {
    	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";
    	(...)
    }
diff mbox series

Patch

diff --git a/src/tplink-safeloader.c b/src/tplink-safeloader.c
index fc46124..0ddbc02 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;
+};
+
 /** 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,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);
 
 	char *s = (char *)entry.data, *end = (char *)(s+entry.size);
 
@@ -3018,14 +3030,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 +3067,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);
 }
 
@@ -3306,6 +3318,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];
 
 	for (i = 0; info->partitions[i].name; i++) {
 		if (!strcmp(info->partitions[i].name, "firmware"))
@@ -3330,7 +3344,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;
+		}
+
 		file_system_partition->base = firmware_partition->base + kernel.st_size;
 
 		/* Align partition start to erase blocks for factory images only */
@@ -3339,15 +3359,38 @@  static void build_image(const char *output,
 
 		file_system_partition->size = firmware_partition->size - file_system_partition->base;
 
-		os_image_partition->name = "os-image";
+		if (info->partition_names.os_image == NULL)
+			os_image_partition->name = "os-image";
+		else {
+			strcpy(os_name, info->partition_names.os_image);
+			os_image_partition->name = os_name;
+		}
+
 		os_image_partition->size = kernel.st_size;
 	}
 
-	parts[0] = make_partition_table(info->partitions);
-	parts[1] = make_soft_version(info, rev);
+	if (info->partition_names.partition_table == NULL)
+		parts[0] = make_partition_table("partition-table", info->partitions);
+	else
+		parts[0] = make_partition_table(info->partition_names.partition_table, info->partitions);
+
+	if (info->partition_names.soft_ver == NULL)
+		parts[1] = make_soft_version("soft-version", info, rev);
+	else
+		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);
+
+	if (info->partition_names.os_image == NULL)
+		parts[3] = read_file("os-image", kernel_image, false, NULL);
+	else
+		parts[3] = read_file(info->partition_names.os_image, kernel_image, false, NULL);
+
+	if (info->partition_names.file_system == NULL)
+		parts[4] = read_file("file-system", rootfs_image, add_jffs2_eof, file_system_partition);
+	else
+		parts[4] = read_file(info->partition_names.file_system, rootfs_image, add_jffs2_eof, file_system_partition);
+
 
 	/* Some devices need the extra-para partition to accept the firmware */
 	if (strcasecmp(info->id, "ARCHER-A6-V3") == 0 ||