[U-Boot] [PATCH v2 0/5] solve issues in gpt management

[U-Boot] [PATCH v2 0/5] solve issues in gpt management

[Patrick Delaunay]

in the last version v2017.09, I see some regression for the command

$> gpt write mmc 0 "name=test,start=0x4400,size=0"
$> gpt write mmc 0 "name=test,size=0"

I use sandbox python test to verify if this issue is also present in
v2017.11-rc1 and when I check the log tests, I detect a other issue
for the swap / rename feature : the offset and the size is always 1MB
align, that cause issue if the partition wasn't initially 1MB align.
And it is the case of the test (the size of partition change after the
command gpt rename or swap)

I propose this patch-set with:
- updated gpt test to highlight the issues
- my proposed correction for the 2 issues


Changes in v2:
- Split test to functional change

Patrick Delaunay (5):
  test/py: gpt: copy persistent file
  test/py: gpt: add test for sub-command read/verify/write
  disk: efi: correct the overlap check on GPT header and PTE
  test/py: gpt: test start LBA for sub-command rename and swap
  cmd: gpt: solve issue for swap

 cmd/gpt.c                 | 12 +++----
 disk/part_efi.c           |  4 +--
 test/py/tests/test_gpt.py | 80 +++++++++++++++++++++++++++++++++++++++--------
 3 files changed, 75 insertions(+), 21 deletions(-)

-- 
2.7.4

[U-Boot] [PATCH v2 1/5] test/py: gpt: copy persistent file

[Patrick Delaunay]

copy the persistent gpt binary file as it can be modified during the test
that avoid issue if the test fail: the test always restart with clean file

Signed-off-by: Patrick Delaunay <patrick.delaunay@st.com>
---

Changes in v2:
- Split test to functional change

 test/py/tests/test_gpt.py | 23 ++++++++++++++---------
 1 file changed, 14 insertions(+), 9 deletions(-)

diff --git a/test/py/tests/test_gpt.py b/test/py/tests/test_gpt.py
index ec25fbb..e58bf61 100644
--- a/test/py/tests/test_gpt.py
+++ b/test/py/tests/test_gpt.py
@@ -28,26 +28,31 @@ class GptTestDiskImage(object):
         """
 
         filename = 'test_gpt_disk_image.bin'
-        self.path = u_boot_console.config.persistent_data_dir + '/' + filename
 
-        if os.path.exists(self.path):
-            u_boot_console.log.action('Disk image file ' + self.path +
+        persistent = u_boot_console.config.persistent_data_dir + '/' + filename
+        self.path = u_boot_console.config.result_dir  + '/' + filename
+
+        if os.path.exists(persistent):
+            u_boot_console.log.action('Disk image file ' + persistent +
                 ' already exists')
         else:
-            u_boot_console.log.action('Generating ' + self.path)
-            fd = os.open(self.path, os.O_RDWR | os.O_CREAT)
+            u_boot_console.log.action('Generating ' + persistent)
+            fd = os.open(persistent, os.O_RDWR | os.O_CREAT)
             os.ftruncate(fd, 4194304)
             os.close(fd)
             cmd = ('sgdisk', '-U', '375a56f7-d6c9-4e81-b5f0-09d41ca89efe',
-                self.path)
+                persistent)
             u_boot_utils.run_and_log(u_boot_console, cmd)
-            cmd = ('sgdisk', '--new=1:2048:2560', self.path)
+            cmd = ('sgdisk', '--new=1:2048:2560', persistent)
             u_boot_utils.run_and_log(u_boot_console, cmd)
-            cmd = ('sgdisk', '--new=2:4096:4608', self.path)
+            cmd = ('sgdisk', '--new=2:4096:4608', persistent)
             u_boot_utils.run_and_log(u_boot_console, cmd)
-            cmd = ('sgdisk', '-l', self.path)
+            cmd = ('sgdisk', '-l', persistent)
             u_boot_utils.run_and_log(u_boot_console, cmd)
 
+        cmd = ('cp', persistent, self.path)
+        u_boot_utils.run_and_log(u_boot_console, cmd)
+
 gtdi = None
 @pytest.fixture(scope='function')
 def state_disk_image(u_boot_console):
-- 
2.7.4

[U-Boot] [PATCH v2 1/5] test/py: gpt: copy persistent file

[Stephen Warren]

On 10/16/2017 10:17 AM, Patrick Delaunay wrote:
> copy the persistent gpt binary file as it can be modified during the test
> that avoid issue if the test fail: the test always restart with clean file

Acked-by: Stephen Warren <swarren@nvidia.com>

[U-Boot] [PATCH v2 2/5] test/py: gpt: add test for sub-command read/verify/write

[Patrick Delaunay]

Signed-off-by: Patrick Delaunay <patrick.delaunay@st.com>
---
The write test failed on error
"Writing GPT: Partition overlap"

./test/py/test.py -k gpt --build

this regression is corrected in next commit of the patchset

Changes in v2: None

 test/py/tests/test_gpt.py | 45 +++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 45 insertions(+)

diff --git a/test/py/tests/test_gpt.py b/test/py/tests/test_gpt.py
index e58bf61..8d5980c 100644
--- a/test/py/tests/test_gpt.py
+++ b/test/py/tests/test_gpt.py
@@ -68,6 +68,31 @@ def state_disk_image(u_boot_console):
 
 @pytest.mark.boardspec('sandbox')
 @pytest.mark.buildconfigspec('cmd_gpt')
+ at pytest.mark.buildconfigspec('cmd_part')
+ at pytest.mark.requiredtool('sgdisk')
+def test_gpt_read(state_disk_image, u_boot_console):
+    """Test the gpt read command."""
+
+    u_boot_console.run_command('host bind 0 ' + state_disk_image.path)
+    output = u_boot_console.run_command('gpt read host 0')
+    assert 'Start 1MiB, size 0MiB' in output
+    assert 'Start 2MiB, size 0MiB' in output
+    output = u_boot_console.run_command('part list host 0')
+    assert '0x00000800	0x00000a00	""' in output
+    assert '0x00001000	0x00001200	""' in output
+
+ at pytest.mark.boardspec('sandbox')
+ at pytest.mark.buildconfigspec('cmd_gpt')
+ at pytest.mark.requiredtool('sgdisk')
+def test_gpt_verify(state_disk_image, u_boot_console):
+    """Test the gpt verify command."""
+
+    u_boot_console.run_command('host bind 0 ' + state_disk_image.path)
+    output = u_boot_console.run_command('gpt verify host 0')
+    assert 'Verify GPT: success!' in output
+
+ at pytest.mark.boardspec('sandbox')
+ at pytest.mark.buildconfigspec('cmd_gpt')
 @pytest.mark.requiredtool('sgdisk')
 def test_gpt_guid(state_disk_image, u_boot_console):
     """Test the gpt guid command."""
@@ -120,3 +145,23 @@ def test_gpt_swap_partitions(state_disk_image, u_boot_console):
     output = u_boot_console.run_command('part list host 0')
     assert '0x000007ff	"second"' in output
     assert '0x000017ff	"first"' in output
+
+ at pytest.mark.boardspec('sandbox')
+ at pytest.mark.buildconfigspec('cmd_gpt')
+ at pytest.mark.buildconfigspec('cmd_part')
+ at pytest.mark.requiredtool('sgdisk')
+def test_gpt_write(state_disk_image, u_boot_console):
+    """Test the gpt write command."""
+
+    u_boot_console.run_command('host bind 0 ' + state_disk_image.path)
+    output = u_boot_console.run_command('gpt write host 0 "name=all,size=0"')
+    assert 'Writing GPT: success!' in output
+    output = u_boot_console.run_command('part list host 0')
+    assert '0x00000022	0x00001fde	"all"' in output
+    output = u_boot_console.run_command('gpt write host 0 "uuid_disk=375a56f7-d6c9-4e81-b5f0-09d41ca89efe;name=first,start=0x100000,size=0x40200;name=second,start=0x200000,size=0x40200;"')
+    assert 'Writing GPT: success!' in output
+    output = u_boot_console.run_command('part list host 0')
+    assert '0x00000800	0x00000a00	"first"' in output
+    assert '0x00001000	0x00001200	"second"' in output
+    output = u_boot_console.run_command('gpt guid host 0')
+    assert '375a56f7-d6c9-4e81-b5f0-09d41ca89efe' in output
-- 
2.7.4

[U-Boot] [PATCH v2 2/5] test/py: gpt: add test for sub-command read/verify/write

[Stephen Warren]

On 10/16/2017 10:17 AM, Patrick Delaunay wrote:
> Signed-off-by: Patrick Delaunay <patrick.delaunay@st.com>

There needs to be a patch description here, so anyone looking at git 
history knows what this patch is intended to do.

> ---
> The write test failed on error
> "Writing GPT: Partition overlap"
> 
> ./test/py/test.py -k gpt --build
> 
> this regression is corrected in next commit of the patchset

Does this mean the test doesn't pass with this patch applied? That's 
wrong. All tests should pass at any git commit. Instead, add this test 
and validate the current expected data (even if it's wrong), and put a 
comment in the code and commit description describing the issue. Then, 
in the commit that fixes the bug in the code, also update the test to 
validate the new (now correct) expected data.

Or, add the new tests after the code is fixed.

> diff --git a/test/py/tests/test_gpt.py b/test/py/tests/test_gpt.py

> @@ -68,6 +68,31 @@ def state_disk_image(u_boot_console):
>   
>  @pytest.mark.boardspec('sandbox')
>  @pytest.mark.buildconfigspec('cmd_gpt')
> + at pytest.mark.buildconfigspec('cmd_part')
> + at pytest.mark.requiredtool('sgdisk')
> +def test_gpt_read(state_disk_image, u_boot_console):
> +    """Test the gpt read command."""
> +
> +    u_boot_console.run_command('host bind 0 ' + state_disk_image.path)
> +    output = u_boot_console.run_command('gpt read host 0')
> +    assert 'Start 1MiB, size 0MiB' in output
> +    assert 'Start 2MiB, size 0MiB' in output

Can those commands include that partion name/number/... in the text they 
assert is in the output? That way, the test will verify that the 
expected/correct partition has the expected start/size, not simply that 
/some/ partition has the expected start/size.

> +    output = u_boot_console.run_command('part list host 0')
> +    assert '0x00000800	0x00000a00	""' in output
> +    assert '0x00001000	0x00001200	""' in output

Do the partitions not have names in the test GPT image? I ask because of 
the empty "" in the text that's verified above. Perhaps we can modify 
the code that creates the GPT image to give the partitions names, so 
that they can be verified here.

[U-Boot] [PATCH v2 3/5] disk: efi: correct the overlap check on GPT header and PTE

[Patrick Delaunay]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="windows-1254", Size: 1812 bytes --]

the partition starting at 0x4400 is refused with overlap error:
  $> gpt write mmc 0 "name=test,start=0x4400,size=0"
  Writing GPT: Partition overlap
  error!

even if the 0x4400 is the first available offset for LBA35 with default
value:
- MBR=LBA1
- GPT header=LBA2
- PTE= 32 LBAs (128 entry), 3 to 34

And the command to have one partition for all the disk failed also :
  $> gpt write mmc 0 "name=test,size=0"

After the patch :

  $> gpt write mmc 0 "name=test,size=0"
  Writing GPT: success!
  $> part list mmc 0

  Partition Map for MMC device 0  --   Partition Type: EFI

  Part	Start LBA	End LBA		Name
	Attributes
	Type GUID
	Partition GUID
  1	0x00000022	0x01ce9fde	"test"
	attrs:	0x0000000000000000
	type:	ebd0a0a2-b9e5-4433-87c0-68b6b72699c7
	type:	data
	guid:	b4b84b8a-04e3-4000-0036-aff5c9c495b1

And 0x22 = 34 LBA => offset = 0x4400 is accepted as expected

Reviewed-by: Łukasz Majewski <lukma@denx.de>
Signed-off-by: Patrick Delaunay <patrick.delaunay@st.com>
---
gpt test are now OK
./test/py/test.py -k gpt --build

test/py/tests/test_gpt.py .......
=> 7 passed, 228 deselected in 1.11 seconds

Changes in v2: None

 disk/part_efi.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/disk/part_efi.c b/disk/part_efi.c
index 0abf487..2992d9e 100644
--- a/disk/part_efi.c
+++ b/disk/part_efi.c
@@ -469,8 +469,8 @@ int gpt_fill_pte(struct blk_desc *dev_desc,
 		 * If our partition overlaps with either the GPT
 		 * header, or the partition entry, reject it.
 		 */
-		if (((start <= hdr_end && hdr_start <= (start + size)) ||
-		     (start <= pte_end && pte_start <= (start + size)))) {
+		if (((start < hdr_end && hdr_start < (start + size)) ||
+		     (start < pte_end && pte_start < (start + size)))) {
 			printf("Partition overlap\n");
 			return -1;
 		}
-- 
2.7.4

[U-Boot] [PATCH v2 3/5] disk: efi: correct the overlap check on GPT header and PTE

[Simon Glass]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="windows-1254", Size: 1501 bytes --]

On 16 October 2017 at 18:17, Patrick Delaunay <patrick.delaunay@st.com> wrote:
> the partition starting at 0x4400 is refused with overlap error:
>   $> gpt write mmc 0 "name=test,start=0x4400,size=0"
>   Writing GPT: Partition overlap
>   error!
>
> even if the 0x4400 is the first available offset for LBA35 with default
> value:
> - MBR=LBA1
> - GPT header=LBA2
> - PTE= 32 LBAs (128 entry), 3 to 34
>
> And the command to have one partition for all the disk failed also :
>   $> gpt write mmc 0 "name=test,size=0"
>
> After the patch :
>
>   $> gpt write mmc 0 "name=test,size=0"
>   Writing GPT: success!
>   $> part list mmc 0
>
>   Partition Map for MMC device 0  --   Partition Type: EFI
>
>   Part  Start LBA       End LBA         Name
>         Attributes
>         Type GUID
>         Partition GUID
>   1     0x00000022      0x01ce9fde      "test"
>         attrs:  0x0000000000000000
>         type:   ebd0a0a2-b9e5-4433-87c0-68b6b72699c7
>         type:   data
>         guid:   b4b84b8a-04e3-4000-0036-aff5c9c495b1
>
> And 0x22 = 34 LBA => offset = 0x4400 is accepted as expected
>
> Reviewed-by: Łukasz Majewski <lukma@denx.de>
> Signed-off-by: Patrick Delaunay <patrick.delaunay@st.com>
> ---
> gpt test are now OK
> ./test/py/test.py -k gpt --build
>
> test/py/tests/test_gpt.py .......
> => 7 passed, 228 deselected in 1.11 seconds
>
> Changes in v2: None
>
>  disk/part_efi.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)

Reviewed-by: Simon Glass <sjg@chromium.org>

[U-Boot] [PATCH v2 4/5] test/py: gpt: test start LBA for sub-command rename and swap

[Patrick Delaunay]

Add test of first and last LBA in gpt for rename and swap.
Only the name is expected to change, so test 3 columns
for part command
1: first LBA (start)
2: last LBA (end)
3: partition name

Signed-off-by: Patrick Delaunay <patrick.delaunay@st.com>
---
WARNING: the last LBA are invalid after rename

before rename
0x00000800  0x00000a00      ""
0x00001000  0x00001200      ""

after rename, the last LBA change
=> 7ff for first = invalid (<start)
=> 17ff for second (size increase)

0x00000800	0x000007ff "first"
0x00001000	0x000017ff	"second"

the issue is corrected in next commit of the patchset


Changes in v2: None

 test/py/tests/test_gpt.py | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

diff --git a/test/py/tests/test_gpt.py b/test/py/tests/test_gpt.py
index 8d5980c..2554f1f 100644
--- a/test/py/tests/test_gpt.py
+++ b/test/py/tests/test_gpt.py
@@ -117,6 +117,7 @@ def test_gpt_save_guid(state_disk_image, u_boot_console):
 @pytest.mark.boardspec('sandbox')
 @pytest.mark.buildconfigspec('cmd_gpt')
 @pytest.mark.buildconfigspec('cmd_gpt_rename')
+ at pytest.mark.buildconfigspec('cmd_part')
 @pytest.mark.requiredtool('sgdisk')
 def test_gpt_rename_partition(state_disk_image, u_boot_console):
     """Test the gpt rename command to write partition names."""
@@ -128,6 +129,9 @@ def test_gpt_rename_partition(state_disk_image, u_boot_console):
     u_boot_console.run_command('gpt rename host 0 2 second')
     output = u_boot_console.run_command('gpt read host 0')
     assert 'name second' in output
+    output = u_boot_console.run_command('part list host 0')
+    assert '0x00000800	0x000007ff	"first"' in output
+    assert '0x00001000	0x000017ff	"second"' in output
 
 @pytest.mark.boardspec('sandbox')
 @pytest.mark.buildconfigspec('cmd_gpt')
@@ -139,12 +143,12 @@ def test_gpt_swap_partitions(state_disk_image, u_boot_console):
 
     u_boot_console.run_command('host bind 0 ' + state_disk_image.path)
     output = u_boot_console.run_command('part list host 0')
-    assert '0x000007ff	"first"' in output
-    assert '0x000017ff	"second"' in output
+    assert '0x00000800	0x000007ff	"first"' in output
+    assert '0x00001000	0x000017ff	"second"' in output
     u_boot_console.run_command('gpt swap host 0 first second')
     output = u_boot_console.run_command('part list host 0')
-    assert '0x000007ff	"second"' in output
-    assert '0x000017ff	"first"' in output
+    assert '0x00000800	0x000007ff	"second"' in output
+    assert '0x00001000	0x000017ff	"first"' in output
 
 @pytest.mark.boardspec('sandbox')
 @pytest.mark.buildconfigspec('cmd_gpt')
-- 
2.7.4

[U-Boot] [PATCH v2 4/5] test/py: gpt: test start LBA for sub-command rename and swap

[Stephen Warren]

On 10/16/2017 10:17 AM, Patrick Delaunay wrote:
> Add test of first and last LBA in gpt for rename and swap.
> Only the name is expected to change, so test 3 columns
> for part command
> 1: first LBA (start)
> 2: last LBA (end)
> 3: partition name
> 
> Signed-off-by: Patrick Delaunay <patrick.delaunay@st.com>
> ---
> WARNING: the last LBA are invalid after rename
> 
> before rename
> 0x00000800  0x00000a00      ""
> 0x00001000  0x00001200      ""
> 
> after rename, the last LBA change
> => 7ff for first = invalid (<start)
> => 17ff for second (size increase)
> 
> 0x00000800	0x000007ff "first"
> 0x00001000	0x000017ff	"second"
> 
> the issue is corrected in next commit of the patchset

That issue should be described in the commit description, and also as a 
comment in the code.

[U-Boot] [PATCH v2 5/5] cmd: gpt: solve issue for swap

[Patrick Delaunay]

don't use prettyprint_part_size() in create_gpt_partitions_list()
that avoid to align offset and size to 1 MiB and increase precision for
start and size
This patch avoid the risk to change partition size
and lost data during swap

Signed-off-by: Patrick Delaunay <patrick.delaunay@st.com>
---

Changes in v2: None

 cmd/gpt.c                 | 12 ++++++------
 test/py/tests/test_gpt.py | 12 ++++++------
 2 files changed, 12 insertions(+), 12 deletions(-)

diff --git a/cmd/gpt.c b/cmd/gpt.c
index 27dd987..707d861 100644
--- a/cmd/gpt.c
+++ b/cmd/gpt.c
@@ -282,14 +282,14 @@ static int create_gpt_partitions_list(int numparts, const char *guid,
 		strcat(partitions_list, "name=");
 		strncat(partitions_list, (const char *)curr->gpt_part_info.name,
 			PART_NAME_LEN + 1);
-		strcat(partitions_list, ",start=");
-		prettyprint_part_size(partstr, (unsigned long)curr->gpt_part_info.start,
-				      (unsigned long) curr->gpt_part_info.blksz);
+		sprintf(partstr, ",start=0x%llx",
+			(unsigned long long)curr->gpt_part_info.start *
+					    curr->gpt_part_info.blksz);
 		/* one extra byte for NULL */
 		strncat(partitions_list, partstr, PART_NAME_LEN + 1);
-		strcat(partitions_list, ",size=");
-		prettyprint_part_size(partstr, curr->gpt_part_info.size,
-				      curr->gpt_part_info.blksz);
+		sprintf(partstr, ",size=0x%llx",
+			(unsigned long long)curr->gpt_part_info.size *
+					    curr->gpt_part_info.blksz);
 		strncat(partitions_list, partstr, PART_NAME_LEN + 1);
 
 		strcat(partitions_list, ",uuid=");
diff --git a/test/py/tests/test_gpt.py b/test/py/tests/test_gpt.py
index 2554f1f..74bc81c 100644
--- a/test/py/tests/test_gpt.py
+++ b/test/py/tests/test_gpt.py
@@ -130,8 +130,8 @@ def test_gpt_rename_partition(state_disk_image, u_boot_console):
     output = u_boot_console.run_command('gpt read host 0')
     assert 'name second' in output
     output = u_boot_console.run_command('part list host 0')
-    assert '0x00000800	0x000007ff	"first"' in output
-    assert '0x00001000	0x000017ff	"second"' in output
+    assert '0x00000800	0x00000a00	"first"' in output
+    assert '0x00001000	0x00001200	"second"' in output
 
 @pytest.mark.boardspec('sandbox')
 @pytest.mark.buildconfigspec('cmd_gpt')
@@ -143,12 +143,12 @@ def test_gpt_swap_partitions(state_disk_image, u_boot_console):
 
     u_boot_console.run_command('host bind 0 ' + state_disk_image.path)
     output = u_boot_console.run_command('part list host 0')
-    assert '0x00000800	0x000007ff	"first"' in output
-    assert '0x00001000	0x000017ff	"second"' in output
+    assert '0x00000800	0x00000a00	"first"' in output
+    assert '0x00001000	0x00001200	"second"' in output
     u_boot_console.run_command('gpt swap host 0 first second')
     output = u_boot_console.run_command('part list host 0')
-    assert '0x00000800	0x000007ff	"second"' in output
-    assert '0x00001000	0x000017ff	"first"' in output
+    assert '0x00000800	0x00000a00	"second"' in output
+    assert '0x00001000	0x00001200	"first"' in output
 
 @pytest.mark.boardspec('sandbox')
 @pytest.mark.buildconfigspec('cmd_gpt')
-- 
2.7.4

[U-Boot] [PATCH v2 5/5] cmd: gpt: solve issue for swap

[Simon Glass]

On 16 October 2017 at 18:17, Patrick Delaunay <patrick.delaunay@st.com> wrote:
> don't use prettyprint_part_size() in create_gpt_partitions_list()
> that avoid to align offset and size to 1 MiB and increase precision for
> start and size
> This patch avoid the risk to change partition size
> and lost data during swap
>
> Signed-off-by: Patrick Delaunay <patrick.delaunay@st.com>
> ---
>
> Changes in v2: None
>
>  cmd/gpt.c                 | 12 ++++++------
>  test/py/tests/test_gpt.py | 12 ++++++------
>  2 files changed, 12 insertions(+), 12 deletions(-)

Reviewed-by: Simon Glass <sjg@chromium.org>