[PATCH] Patch to handle partitions with alternate names.

[Sander Vanheule]

Hi Ole Kristian,

On Sun, 2022-05-01 at 23:12 +0200, Ole Kristian Lona wrote:
> 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.

I saved your mail as wrapped by the mailing list, and then ran checkpatch.pl on that
(mbox) file. It is possible that's what causing these line endings to show up. If you can
update your SPF record to be more compatible with mailing lists, that would help to
prevent this.

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

Aha, flash_partition_entry.name is a char *. That would indeed discard const-ness. A
cleaner solution perhaps, is to change flash_partition_entry.name to const char *.

> > 
> > >  
> > >         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";

This is currently assigning a (static) const char * to a char * variable, which is already
an issue. I'm pretty sure that trying to modify the static buffer containing "file-system"
would be a bad idea. add_flash_partition() would also need to be updated, and possibly
other functions as well.

If you send an update with a patch converting flash_partition_entry.name to const char *,
that should be a separate patch preceeding this one.

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

Kernel-style coding omits the braces on single-line conditional blocks, so the same is
done here. Although generally speaking tplink-safeloader doesn't really adhere to the
kernel's code style guidelines, that doesn't mean we can't do better for
additions/changes. :-)


Best,
Sander