[U-Boot] [PATCH v2 0/5] solve issues in gpt management
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(-)
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):
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
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') +@pytest.mark.buildconfigspec('cmd_part') +@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 + +@pytest.mark.boardspec('sandbox') +@pytest.mark.buildconfigspec('cmd_gpt') +@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 + +@pytest.mark.boardspec('sandbox') +@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 + +@pytest.mark.boardspec('sandbox') +@pytest.mark.buildconfigspec('cmd_gpt') +@pytest.mark.buildconfigspec('cmd_part') +@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
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') +@pytest.mark.buildconfigspec('cmd_part') +@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.
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; }
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
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') +@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')
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.
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')
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
participants (3)
-
Patrick Delaunay -
Simon Glass -
Stephen Warren