[U-Boot] [PATCH 2/2] tools/env: allow negative offsets

[Stefan Agner]

On 2016-07-17 06:37, Andreas Fenkart wrote:
> Hi,
> 
> 2016-07-14 2:14 GMT+02:00 Stefan Agner <stefan at agner.ch>:
>> From: Stefan Agner <stefan.agner at toradex.com>
>>
>> A negative value for the offset is treated as a backwards offset for
>> from the end of the device/partition for block devices. This aligns
>> the behavior of the config file with the syntax of CONFIG_ENV_OFFSET
>> where the functionality has been introduced with
>> commit 5c088ee841f9 ("env_mmc: allow negative CONFIG_ENV_OFFSET").
>>
>> Signed-off-by: Stefan Agner <stefan.agner at toradex.com>
> lgtm
> 
>> ---
>>
>>  tools/env/fw_env.c      | 39 ++++++++++++++++++++++++++++++---------
>>  tools/env/fw_env.config |  5 +++++
>>  2 files changed, 35 insertions(+), 9 deletions(-)
>>
>> diff --git a/tools/env/fw_env.c b/tools/env/fw_env.c
>> index 06d6865..be338a5b 100644
>> --- a/tools/env/fw_env.c
>> +++ b/tools/env/fw_env.c
>> @@ -14,6 +14,7 @@
>>  #include <errno.h>
>>  #include <env_flags.h>
>>  #include <fcntl.h>
>> +#include <linux/fs.h>
>>  #include <linux/stringify.h>
>>  #include <ctype.h>
>>  #include <stdio.h>
>> @@ -51,7 +52,7 @@ struct env_opts default_opts = {
>>
>>  struct envdev_s {
>>         const char *devname;            /* Device name */
>> -       ulong devoff;                   /* Device offset */
>> +       long long devoff;               /* Device offset */
>>         ulong env_size;                 /* environment size */
>>         ulong erase_size;               /* device erase size */
>>         ulong env_sectors;              /* number of environment sectors */
>> @@ -994,7 +995,7 @@ static int flash_write (int fd_current, int fd_target, int dev_target)
>>         }
>>
>>  #ifdef DEBUG
>> -       fprintf(stderr, "Writing new environment at 0x%lx on %s\n",
>> +       fprintf(stderr, "Writing new environment at 0x%llx on %s\n",
>>                 DEVOFFSET (dev_target), DEVNAME (dev_target));
>>  #endif
>>
>> @@ -1010,7 +1011,7 @@ static int flash_write (int fd_current, int fd_target, int dev_target)
>>                         offsetof (struct env_image_redundant, flags);
>>  #ifdef DEBUG
>>                 fprintf(stderr,
>> -                       "Setting obsolete flag in environment at 0x%lx on %s\n",
>> +                       "Setting obsolete flag in environment at 0x%llx on %s\n",
>>                         DEVOFFSET (dev_current), DEVNAME (dev_current));
>>  #endif
>>                 flash_flag_obsolete (dev_current, fd_current, offset);
>> @@ -1335,7 +1336,27 @@ static int check_device_config(int dev)
>>                 }
>>                 DEVTYPE(dev) = mtdinfo.type;
>>         } else {
>> +               uint64_t size;
>>                 DEVTYPE(dev) = MTD_ABSENT;
>> +
>> +               /*
>> +                * Check for negative offsets, treat it as backwards offset
>> +                * from the end of the block device
>> +                */
>> +               if (DEVOFFSET(dev) < 0) {
>> +                       rc = ioctl(fd, BLKGETSIZE64, &size);
>> +                       if (rc < 0) {
>> +                               fprintf(stderr, "Could not get block device size on %s\n",
>> +                                       DEVNAME(dev));
>> +                               goto err;
>> +                       }
>> +
>> +                       DEVOFFSET(dev) = DEVOFFSET(dev) + size;
> 
> I wonder if we need a check, that the redundant areas don't overlap.
> But we can add that later

We haven't had it before, but yeah I agree it would be a sensible thing
to do. And we have now a good place to add such checks...

--
Stefan

> 
> 
>> +#ifdef DEBUG
>> +                       fprintf(stderr, "Calculated device offset 0x%llx on %s\n",
>> +                               DEVOFFSET(dev), DEVNAME(dev));
>> +#endif
>> +               }
>>         }
>>
>>  err:
>> @@ -1434,12 +1455,12 @@ static int get_config (char *fname)
>>                 if (dump[0] == '#')
>>                         continue;
>>
>> -               rc = sscanf (dump, "%ms %lx %lx %lx %lx",
>> -                            &devname,
>> -                            &DEVOFFSET (i),
>> -                            &ENVSIZE (i),
>> -                            &DEVESIZE (i),
>> -                            &ENVSECTORS (i));
>> +               rc = sscanf(dump, "%ms %lli %lx %lx %lx",
>> +                           &devname,
>> +                           &DEVOFFSET(i),
>> +                           &ENVSIZE(i),
>> +                           &DEVESIZE(i),
>> +                           &ENVSECTORS(i));
>>
>>                 if (rc < 3)
>>                         continue;
>> diff --git a/tools/env/fw_env.config b/tools/env/fw_env.config
>> index 6f216f9..f0f66aa 100644
>> --- a/tools/env/fw_env.config
>> +++ b/tools/env/fw_env.config
>> @@ -4,6 +4,7 @@
>>  # Notice, that the "Number of sectors" is not required on NOR and SPI-dataflash.
>>  # Futhermore, if the Flash sector size is ommitted, this value is assumed to
>>  # be the same as the Environment size, which is valid for NOR and SPI-dataflash
>> +# Device offset must be prefixed with 0x to be parsed as a hexadecimal value.
>>
>>  # NOR example
>>  # MTD device name      Device offset   Env. size       Flash sector size       Number of sectors
>> @@ -18,8 +19,12 @@
>>  # NAND example
>>  #/dev/mtd0             0x4000          0x4000          0x20000                 2
>>
>> +# On a block device a negative offset is treated as a backwards offset from the
>> +# end of the device/partition, rather than a forwards offset from the start.
>> +
>>  # Block device example
>>  #/dev/mmcblk0          0xc0000         0x20000
>> +#/dev/mmcblk0          -0x20000        0x20000
>>
>>  # VFAT example
>>  #/boot/uboot.env       0x0000          0x4000
>> --
>> 2.9.0
>>