Re: [U-Boot] [PATCH v6 3/3] GPT: provide commands to selectively rename partitions

[Lothar Waßmann]

Hi,

> The underlying cause of the problem is that u-boot's implementations
> of strlen() and the CLI handle strings differently.   The former
>
"u-boot's implementation" is conformant with the standard and well
documented libc implementation that exists since the dawn of C
programming.

> terminates strings only on NULLs, while the latter terminates strings
> *either* on NULLs or on semicolons.Reading the partition table
> back from a device's GPT results in a string with a NULL only at the
> very end, but with one semicolon per partition.   Running 'gpt verify
> mmc 0 $partitions" or 'gpt write mmc 0 $partitions' therefore causes a
> crash if the user has previously typed 'setenv partitions 1;2;3;4;5;'
> rather than 'setenv partitions "1;2;3;4;5;"'.   In the former case,
> the partitions string ends up being '1' without NULL-termination,
>
That's nonsense. _Every_ string in the environment is NULL terminated.
In the former case of your example the characters after the first
semicolon are interpreted as commands and lead to an error message
Unknown command '2' - try 'help'
...
because the ';' is treated as a command terminator by the parser
unless quoted.


Lothar Waßmann
___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot

Re: [U-Boot] [PATCH v6 3/3] GPT: provide commands to selectively rename partitions

[Alison Chaiken]

On Tue, Jun 27, 2017 at 12:05 AM, Lothar Waßmann  
wrote:
>
> Hi,
>
> On Sun, 25 Jun 2017 14:54:56 -0700 Alison Chaiken wrote:
> > On Sun, Jun 18, 2017 at 4:03 AM, Wolfgang Denk  wrote:
> >
> > > Dear Alison,
> > >
> > > In message 

Re: [U-Boot] [PATCH v6 3/3] GPT: provide commands to selectively rename partitions

[Lothar Waßmann]

Hi,

On Tue, 27 Jun 2017 09:05:14 +0200 Lothar Waßmann wrote:
> Hi,
> 
> On Sun, 25 Jun 2017 14:54:56 -0700 Alison Chaiken wrote:
> > On Sun, Jun 18, 2017 at 4:03 AM, Wolfgang Denk  wrote:
> > 
> > > Dear Alison,
> > >
> > > In message 

Re: [U-Boot] [PATCH v6 3/3] GPT: provide commands to selectively rename partitions

[Lothar Waßmann]

Hi,

On Sun, 25 Jun 2017 14:54:56 -0700 Alison Chaiken wrote:
> On Sun, Jun 18, 2017 at 4:03 AM, Wolfgang Denk  wrote:
> 
> > Dear Alison,
> >
> > In message 

Re: [U-Boot] [PATCH v6 3/3] GPT: provide commands to selectively rename partitions

[Tom Rini]

On Sun, Jun 25, 2017 at 02:54:56PM -0700, Alison Chaiken wrote:
> On Sun, Jun 18, 2017 at 4:03 AM, Wolfgang Denk  wrote:
> 
> > Dear Alison,
> >
> > In message 

Re: [U-Boot] [PATCH v6 3/3] GPT: provide commands to selectively rename partitions

[Alison Chaiken]

On Sun, Jun 18, 2017 at 4:03 AM, Wolfgang Denk  wrote:

> Dear Alison,
>
> In message 

Re: [U-Boot] [PATCH v6 3/3] GPT: provide commands to selectively rename partitions

[Wolfgang Denk]

Dear Tom,

In message <20170612145646.GY10782@bill-the-cat> you wrote:
> 
> Looking at the man page for atoi:
> The  atoi() function converts the initial portion of the string pointed
> to by nptr to int.  The behavior is the same as
> 
>   strtol(nptr, NULL, 10);
> 
> So we should just re-work to use simple_strtol here.  And since you
> brought it up on IRC, I suppose at this point a 'clean' posting of all 5
> patches marked as 'v8' might make it easier to see what's what and in
> what order.  Thanks again!

Hm... the claim that the behaviour is the _same_ is not strictly
correct.

atoi() returns "int", while strtol() returns "long int"...

Anyway, you are right that atoi() is not being used anywhere in
active U-Boot code.

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,  Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: w...@denx.de
C++ is the best example of second-system effect since OS/360.
___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot

Re: [U-Boot] [PATCH v6 3/3] GPT: provide commands to selectively rename partitions

[Wolfgang Denk]

Dear Alison,

In message 

Re: [U-Boot] [PATCH v6 3/3] GPT: provide commands to selectively rename partitions

[Tom Rini]

On Mon, Jun 12, 2017 at 07:24:17AM -0700, Alison Chaiken wrote:
> On Mon, Jun 12, 2017 at 12:45 AM, Wolfgang Denk  wrote:
> 
> > Dear Alison,
> >
> > In message <1497137617-772-1-git-send-email-ali...@peloton-tech.com> you
> > wrote:
> > >
> > > This patch provides support in u-boot for renaming GPT
> > > partitions.  The renaming is accomplished via new 'gpt swap'
> > > and 'gpt rename' commands.
> >
> > Thanks.
> >
> > One question: can multiple GPT partitions have the same name?
> 
> The idea behind the 'swap' mode is that a storage device can have two
> sets of partitions, one set all named 'primary' and one set all named
> 'backup'.  The software updater in userspace can then simply rename
> the partitions with sgdisk in order to pick the new image.   The swap
> mode changes the whole set of labels at once, so there's little chance
> of being interrupted.

I'm a little confused here now then.  Can you provide an example set of
usage, in the cover letter for v8 of the series (see below) that makes
it a bit clearer?  Thanks!

> > > The 'swap' mode prints a warning if no matching partition names
> > > are found.  If only one matching name of a provided pair is found, it
> > > renames the matching partitions to the new name.
> >
> > I see a problem here...
> >
> > > + if (!strcmp(subcomm, "swap")) {
> > > + if ((strlen(name1) > PART_NAME_LEN) || (strlen(name2) >
> > PART_NAME_LEN)) {
> > > + printf("Names longer than %d characters are
> > truncated.\n", PART_NAME_LEN);
> > > + return -EINVAL;
> > > + }
> > > + list_for_each(pos, _partitions) {
> > > + curr = list_entry(pos, struct disk_part, list);
> > > + if (!strcmp((char *)curr->gpt_part_info.name,
> > name1)) {
> > > + strcpy((char *)curr->gpt_part_info.name,
> > name2);
> > > + changed++;
> > > + }
> > > + else if (!strcmp((char *)curr->gpt_part_info.name,
> > name2)) {
> > > + strcpy((char *)curr->gpt_part_info.name,
> > name1);
> > > + changed++;
> > > + }
> > > +
> > > + }
> > > + if (changed == 0) {
> > > + printf("No matching partition names were
> > found.\n");
> > > + return ret;
> > > + }
> >
> > You will never know if there really was a pair of names that was
> > swapped.  Just a single rename of name1->name2 _or_ of name2->name1
> > will make the user think everything was fine.
> >
> 
> Point taken.   The last version I posted has two counters instead of just
> changed in order to address this problem.
> 
> 
> 
> > [Note: I'm in the process of relocating and will be offline for the
> > next 2..3. days.  Don't expect more comments from mee soon.
> > Sorry...]
> > Best regards,
> > Wolfgang Denk
> 
> 
> 
> One additional note: the last version I posted worked fine for the sandbox,
> but wouldn't link for an ARM target with the Linaro toolchain, as the
> linker couldn't find atoi().   I guess the libc for the x86 compiler
> includes it.   To test on ARM, I copied in simple_atoi() from
> lib/vsprintf.c, but assuredly that is an ugly solution.Does anyone have
> a better idea to solve this problem?

Looking at the man page for atoi:
The  atoi() function converts the initial portion of the string pointed
to by nptr to int.  The behavior is the same as

strtol(nptr, NULL, 10);

So we should just re-work to use simple_strtol here.  And since you
brought it up on IRC, I suppose at this point a 'clean' posting of all 5
patches marked as 'v8' might make it easier to see what's what and in
what order.  Thanks again!

-- 
Tom


signature.asc
Description: Digital signature
___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot

Re: [U-Boot] [PATCH v6 3/3] GPT: provide commands to selectively rename partitions

[Alison Chaiken]

On Mon, Jun 12, 2017 at 12:45 AM, Wolfgang Denk  wrote:

> Dear Alison,
>
> In message <1497137617-772-1-git-send-email-ali...@peloton-tech.com> you
> wrote:
> >
> > This patch provides support in u-boot for renaming GPT
> > partitions.  The renaming is accomplished via new 'gpt swap'
> > and 'gpt rename' commands.
>
> Thanks.
>
> One question: can multiple GPT partitions have the same name?
>


The idea behind the 'swap' mode is that a storage device can have two sets
of partitions, one set all named 'primary' and one set all named 'backup'.
  The software updater in userspace can then simply rename the partitions
with sgdisk in order to pick the new image.   The swap mode changes the
whole set of labels at once, so there's little chance of being interrupted.


>
> > The 'swap' mode prints a warning if no matching partition names
> > are found.  If only one matching name of a provided pair is found, it
> > renames the matching partitions to the new name.
>
> I see a problem here...
>
> > + if (!strcmp(subcomm, "swap")) {
> > + if ((strlen(name1) > PART_NAME_LEN) || (strlen(name2) >
> PART_NAME_LEN)) {
> > + printf("Names longer than %d characters are
> truncated.\n", PART_NAME_LEN);
> > + return -EINVAL;
> > + }
> > + list_for_each(pos, _partitions) {
> > + curr = list_entry(pos, struct disk_part, list);
> > + if (!strcmp((char *)curr->gpt_part_info.name,
> name1)) {
> > + strcpy((char *)curr->gpt_part_info.name,
> name2);
> > + changed++;
> > + }
> > + else if (!strcmp((char *)curr->gpt_part_info.name,
> name2)) {
> > + strcpy((char *)curr->gpt_part_info.name,
> name1);
> > + changed++;
> > + }
> > +
> > + }
> > + if (changed == 0) {
> > + printf("No matching partition names were
> found.\n");
> > + return ret;
> > + }
>
> You will never know if there really was a pair of names that was
> swapped.  Just a single rename of name1->name2 _or_ of name2->name1
> will make the user think everything was fine.
>

Point taken.   The last version I posted has two counters instead of just
changed in order to address this problem.



> [Note: I'm in the process of relocating and will be offline for the
> next 2..3. days.  Don't expect more comments from mee soon.
> Sorry...]
> Best regards,
> Wolfgang Denk



One additional note: the last version I posted worked fine for the sandbox,
but wouldn't link for an ARM target with the Linaro toolchain, as the
linker couldn't find atoi().   I guess the libc for the x86 compiler
includes it.   To test on ARM, I copied in simple_atoi() from
lib/vsprintf.c, but assuredly that is an ugly solution.Does anyone have
a better idea to solve this problem?

Thanks,
Alison Chaiken
Peloton Technology
___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot

Re: [U-Boot] [PATCH v6 3/3] GPT: provide commands to selectively rename partitions

[Wolfgang Denk]

Dear Alison,

In message <1497137617-772-1-git-send-email-ali...@peloton-tech.com> you wrote:
> 
> This patch provides support in u-boot for renaming GPT
> partitions.  The renaming is accomplished via new 'gpt swap'
> and 'gpt rename' commands.

Thanks.

One question: can multiple GPT partitions have the same name?

> The 'swap' mode prints a warning if no matching partition names
> are found.  If only one matching name of a provided pair is found, it
> renames the matching partitions to the new name.

I see a problem here...

> + if (!strcmp(subcomm, "swap")) {
> + if ((strlen(name1) > PART_NAME_LEN) || (strlen(name2) > 
> PART_NAME_LEN)) {
> + printf("Names longer than %d characters are 
> truncated.\n", PART_NAME_LEN);
> + return -EINVAL;
> + }
> + list_for_each(pos, _partitions) {
> + curr = list_entry(pos, struct disk_part, list);
> + if (!strcmp((char *)curr->gpt_part_info.name, name1)) {
> + strcpy((char *)curr->gpt_part_info.name, name2);
> + changed++;
> + }
> + else if (!strcmp((char *)curr->gpt_part_info.name, 
> name2)) {
> + strcpy((char *)curr->gpt_part_info.name, name1);
> + changed++;
> + }
> +
> + }
> + if (changed == 0) {
> + printf("No matching partition names were found.\n");
> + return ret;
> + }

You will never know if there really was a pair of names that was
swapped.  Just a single rename of name1->name2 _or_ of name2->name1
will make the user think everything was fine.

[Note: I'm in the process of relocating and will be offline for the
next 2..3. days.  Don't expect more comments from mee soon.
Sorry...]

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,  Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: w...@denx.de
"I dislike companies that have a we-are-the-high-priests-of-hardware-
so-you'll-like-what-we-give-you attitude. I like commodity markets in
which iron-and-silicon hawkers know that they exist to  provide  fast
toys for software types like me to play with..."- Eric S. Raymond
___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot

Re: [U-Boot] [PATCH v6 3/3] GPT: provide commands to selectively rename partitions

[Tom Rini]

On Sat, Jun 10, 2017 at 04:33:37PM -0700, ali...@peloton-tech.com wrote:
> From: Alison Chaiken 
> 
> This patch provides support in u-boot for renaming GPT
> partitions.  The renaming is accomplished via new 'gpt swap'
> and 'gpt rename' commands.
> 
> The 'swap' mode prints a warning if no matching partition names
> are found.  If only one matching name of a provided pair is found, it
> renames the matching partitions to the new name.

So in the case where 'gpt swap partA partB' is used and partB doesn't
exist, it's the same as 'gpt rename partA partB' ?  That seems like it
might surprise users and I think if we changed:

> + if (!strcmp(subcomm, "swap")) {
> + if ((strlen(name1) > PART_NAME_LEN) || (strlen(name2) > 
> PART_NAME_LEN)) {
> + printf("Names longer than %d characters are 
> truncated.\n", PART_NAME_LEN);
> + return -EINVAL;
> + }
> + list_for_each(pos, _partitions) {
> + curr = list_entry(pos, struct disk_part, list);
> + if (!strcmp((char *)curr->gpt_part_info.name, name1)) {
> + strcpy((char *)curr->gpt_part_info.name, name2);
> + changed++;
> + }
> + else if (!strcmp((char *)curr->gpt_part_info.name, 
> name2)) {
> + strcpy((char *)curr->gpt_part_info.name, name1);
> + changed++;
> + }
> +
> + }
> + if (changed == 0) {
> + printf("No matching partition names were found.\n");
> + return ret;
> + }

We could also test for if changed == 1 and return an error.  I assume
that a GPT with the same name used more than one time is already
invalid, but we could be defensive here and test for != 2 instead.
Thanks!

-- 
Tom


signature.asc
Description: Digital signature
___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot

[U-Boot] [PATCH v6 3/3] GPT: provide commands to selectively rename partitions

[alison]

From: Alison Chaiken 

This patch provides support in u-boot for renaming GPT
partitions.  The renaming is accomplished via new 'gpt swap'
and 'gpt rename' commands.

The 'swap' mode prints a warning if no matching partition names
are found.  If only one matching name of a provided pair is found, it
renames the matching partitions to the new name.

Rewriting the partition table has the side-effect that all partitions
end up with "msftdata" flag set.  The reason is that partition type
PARTITION_BASIC_DATA_GUID is hard-coded in the gpt_fill_pte()
function.  This does not appear to cause any harm.

Changes since v5:
-- The former 'gpt flip' is now 'gpt swap', which takes the two
   arbitrary partition names it exchanges as command-line input.
-- A new command, 'gpt rename', now allows renaming of single,
   specified partitions.
-- Updated README.gpt to reflect the changes.

Signed-off-by: Alison Chaiken 
---
 cmd/Kconfig|   8 ++
 cmd/gpt.c  | 239 +++--
 doc/README.gpt |  20 -
 3 files changed, 261 insertions(+), 6 deletions(-)

diff --git a/cmd/Kconfig b/cmd/Kconfig
index 412bf24..7b262de 100644
--- a/cmd/Kconfig
+++ b/cmd/Kconfig
@@ -595,6 +595,14 @@ config CMD_GPT
  Enable the 'gpt' command to ready and write GPT style partition
  tables.
 
+config CMD_GPT_RENAME
+   bool "GPT partition renaming commands"
+   depends on CMD_GPT
+   help
+ Enables the 'gpt' command to interchange names on two GPT
+ partitions via the 'gpt swap' command or to rename single
+ partitions via the 'rename' command.
+
 config CMD_ARMFLASH
#depends on FLASH_CFI_DRIVER
bool "armflash"
diff --git a/cmd/gpt.c b/cmd/gpt.c
index 669031f8..aa3245a 100644
--- a/cmd/gpt.c
+++ b/cmd/gpt.c
@@ -20,6 +20,9 @@
 #include 
 #include 
 #include 
+#include 
+
+extern int atoi(const char *nptr);
 
 static LIST_HEAD(disk_partitions);
 
@@ -194,16 +197,33 @@ static struct disk_part 
*allocate_disk_part(disk_partition_t *info, int partnum)
return newpart;
 }
 
+static void prettyprint_part_size(char *sizestr, unsigned long partsize,
+ unsigned long blksize)
+{
+   unsigned long long partbytes;
+   unsigned long partmegabytes;
+
+   partbytes = partsize * blksize;
+   partmegabytes = lldiv(partbytes, SZ_1M);
+   snprintf(sizestr, 16, "%luMiB", partmegabytes);
+}
+
 static void print_gpt_info(void)
 {
struct list_head *pos;
struct disk_part *curr;
+   char partstartstr[16];
+   char partsizestr[16];
 
list_for_each(pos, _partitions) {
curr = list_entry(pos, struct disk_part, list);
+   prettyprint_part_size(partstartstr, (unsigned 
long)curr->gpt_part_info.start,
+ (unsigned long) 
curr->gpt_part_info.blksz);
+   prettyprint_part_size(partsizestr, (unsigned 
long)curr->gpt_part_info.size,
+ (unsigned long) 
curr->gpt_part_info.blksz);
+
printf("Partition %d:\n", curr->partnum);
-   printf("1st block %x, size %x\n", 
(unsigned)curr->gpt_part_info.start,
-  (unsigned)curr->gpt_part_info.size);
+   printf("Start %s, size %s\n", partstartstr, partsizestr);
printf("Block size %lu, name %s\n", curr->gpt_part_info.blksz,
   curr->gpt_part_info.name);
printf("Type %s, bootable %d\n", curr->gpt_part_info.type,
@@ -215,6 +235,74 @@ static void print_gpt_info(void)
}
 }
 
+#ifdef CONFIG_CMD_GPT_RENAME
+static int calc_parts_list_len(int numparts)
+{
+   int partlistlen = UUID_STR_LEN + 1 + strlen("uuid_disk=");
+   /* for the comma */
+   partlistlen++;
+
+   /* per-partition additions; numparts starts at 1, so this should be 
correct */
+   partlistlen += numparts * (strlen("name=,") + PART_NAME_LEN + 1);
+   /* see part.h for definition of struct disk_partition */
+   partlistlen += numparts * (strlen("start=MiB,") + sizeof(lbaint_t) + 1);
+   partlistlen += numparts * (strlen("size=MiB,") + sizeof(lbaint_t) + 1);
+   partlistlen += numparts * (strlen("uuid=;") + UUID_STR_LEN + 1);
+   /* for the terminating null */
+   partlistlen++;
+   debug("Length of partitions_list is %d for %d partitions\n", 
partlistlen,
+  numparts);
+   return partlistlen;
+}
+
+/*
+ * create the string that upstream 'gpt write' command will accept as an
+ * argument
+ *
+ * From doc/README.gpt, Format of partitions layout:
+ *"uuid_disk=...;name=u-boot,size=60MiB,uuid=...;
+ * name=kernel,size=60MiB,uuid=...;"
+ * The fields 'name' and 'size' are mandatory for every partition.
+ * The field 'start' is optional. The fields 'uuid' and 'uuid_disk'
+ * are optional if CONFIG_RANDOM_UUID is enabled.
+ */
+static int