[U-Boot] [PATCH v3 0/6] 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
tests are ok on v2017.11-rc1
Changes in v3: - update after Stephen Warren comments - Add partition name in persistent data and test them - split test_gpt.py update: commit to add the test write command - tests are now OK for each commit - Indicate LBA end error for rename command in test/py and commit message
Changes in v2: - Split test to functional change
Patrick Delaunay (6): test/py: gpt: copy persistent file test/py: gpt: add test for sub-command read and verify disk: efi: correct the overlap check on GPT header and PTE test/py: gpt: add test for sub-command write test/py: gpt: test start LBA for sub-command rename and swap cmd: gpt: solve issue for swap and rename command
cmd/gpt.c | 12 +++---- disk/part_efi.c | 4 +-- test/py/tests/test_gpt.py | 82 +++++++++++++++++++++++++++++++++++++++-------- 3 files changed, 77 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
Acked-by: Stephen Warren swarren@nvidia.com Signed-off-by: Patrick Delaunay patrick.delaunay@st.com ---
Changes in v3: None 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 Wed, Oct 18, 2017 at 03:11:03PM +0200, 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
Applied to u-boot/master, thanks!
add sandbox test for some gpt sub-command - gpt read / part list : read the gpt partition created by sgdisk on host test start, size, LBA and name output - gpt verify : verify the gpt partition create by sgdisk on host
PS: persistent data test_gpt_disk_image.bin are udpated
Signed-off-by: Patrick Delaunay patrick.delaunay@st.com --- warning: need to erase previously create file ./build-sandbox/persistent-data/test_gpt_disk_image.bin
gpt test are OK on v2017.11-rc2: ./test/py/test.py -k gpt --build
test/py/tests/test_gpt.py ......
=> 6 passed, 229 deselected in 5.12 seconds
Changes in v3: - update after Stephen Warren comments - Add partition name in persistent data and test them
Changes in v2: None
test/py/tests/test_gpt.py | 31 +++++++++++++++++++++++++++++-- 1 file changed, 29 insertions(+), 2 deletions(-)
diff --git a/test/py/tests/test_gpt.py b/test/py/tests/test_gpt.py index e58bf61..5cfbf1f 100644 --- a/test/py/tests/test_gpt.py +++ b/test/py/tests/test_gpt.py @@ -43,9 +43,9 @@ class GptTestDiskImage(object): cmd = ('sgdisk', '-U', '375a56f7-d6c9-4e81-b5f0-09d41ca89efe', persistent) u_boot_utils.run_and_log(u_boot_console, cmd) - cmd = ('sgdisk', '--new=1:2048:2560', persistent) + cmd = ('sgdisk', '--new=1:2048:2560', '-c 1:part1', persistent) u_boot_utils.run_and_log(u_boot_console, cmd) - cmd = ('sgdisk', '--new=2:4096:4608', persistent) + cmd = ('sgdisk', '--new=2:4096:4608', '-c 2:part2', persistent) u_boot_utils.run_and_log(u_boot_console, cmd) cmd = ('sgdisk', '-l', persistent) u_boot_utils.run_and_log(u_boot_console, cmd) @@ -68,6 +68,33 @@ 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 'Block size 512, name part1' in output + assert 'Start 2MiB, size 0MiB' in output + assert 'Block size 512, name part2' in output + output = u_boot_console.run_command('part list host 0') + assert '0x00000800 0x00000a00 "part1"' in output + assert '0x00001000 0x00001200 "part2"' 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."""
On Wed, Oct 18, 2017 at 03:11:04PM +0200, Patrick Delaunay wrote:
add sandbox test for some gpt sub-command
- gpt read / part list : read the gpt partition created by sgdisk on host test start, size, LBA and name output
- gpt verify : verify the gpt partition create by sgdisk on host
PS: persistent data test_gpt_disk_image.bin are udpated
Signed-off-by: Patrick Delaunay patrick.delaunay@st.com
Applied to u-boot/master, thanks!
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 v3: None 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 782f8be..7862bee 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 Wed, Oct 18, 2017 at 03:11:05PM +0200, Patrick Delaunay 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
Applied to u-boot/master, thanks!
+ test write for one partition on all the device (size=0) + test write with disk uuid and 2 partitions
Signed-off-by: Patrick Delaunay patrick.delaunay@st.com --- This write test failed without the previous patch with the error: "Writing GPT: Partition overlap"
gpt test are now OK on v2017.11-rc2: ./test/py/test.py -k gpt --build
test/py/tests/test_gpt.py .......
=> 7 passed, 229 deselected in 5.23 seconds
Changes in v3: - split test_gpt.py update: commit to add the test write command - tests are now OK for each commit
Changes in v2: None
test/py/tests/test_gpt.py | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+)
diff --git a/test/py/tests/test_gpt.py b/test/py/tests/test_gpt.py index 5cfbf1f..2eb07a1 100644 --- a/test/py/tests/test_gpt.py +++ b/test/py/tests/test_gpt.py @@ -147,3 +147,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 Wed, Oct 18, 2017 at 03:11:06PM +0200, Patrick Delaunay wrote:
- test write for one partition on all the device (size=0)
- test write with disk uuid and 2 partitions
Signed-off-by: Patrick Delaunay patrick.delaunay@st.com
Applied to u-boot/master, thanks!
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
After rename, the last LBA change and it is a error in current U-Boot code + "first" = 0x7ff : invalid value (<start) + "second" = 0x17ff => size increasing !
Signed-off-by: Patrick Delaunay patrick.delaunay@st.com ---
Before rename 0x00000800 0x00000a00 "part1" 0x00001000 0x00001200 "part2"
And after rename last LBA are invalid 0x00000800 0x000007ff "first" 0x00001000 0x000017ff "second"
this issue will be corrected in next commit of the patchset
Changes in v3: - Indicate LBA end error for rename command in test/py and commit message
Changes in v2: None
test/py/tests/test_gpt.py | 16 ++++++++++++---- 1 file changed, 12 insertions(+), 4 deletions(-)
diff --git a/test/py/tests/test_gpt.py b/test/py/tests/test_gpt.py index 2eb07a1..b7adc10 100644 --- a/test/py/tests/test_gpt.py +++ b/test/py/tests/test_gpt.py @@ -119,6 +119,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.""" @@ -130,6 +131,13 @@ 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 + # command error here because 'end LBA' (column 2) change after rename + # (previous value can be found in test_gpt_read) + # "first" 0xa00 => 0x7ff : it is an invalid value < start LBA ! + # "seconf" 0x1200 => 0x17ff : size is increasing !
@pytest.mark.boardspec('sandbox') @pytest.mark.buildconfigspec('cmd_gpt') @@ -141,12 +149,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 Wed, Oct 18, 2017 at 03:11:07PM +0200, 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
After rename, the last LBA change and it is a error in current U-Boot code
- "first" = 0x7ff : invalid value (<start)
- "second" = 0x17ff => size increasing !
Signed-off-by: Patrick Delaunay patrick.delaunay@st.com
Applied to u-boot/master, thanks!
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 rename or swap.
Signed-off-by: Patrick Delaunay patrick.delaunay@st.com ---
Changes in v3: None Changes in v2: None
cmd/gpt.c | 12 ++++++------ test/py/tests/test_gpt.py | 16 ++++++---------- 2 files changed, 12 insertions(+), 16 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 b7adc10..b9b5e5f 100644 --- a/test/py/tests/test_gpt.py +++ b/test/py/tests/test_gpt.py @@ -132,12 +132,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 - # command error here because 'end LBA' (column 2) change after rename - # (previous value can be found in test_gpt_read) - # "first" 0xa00 => 0x7ff : it is an invalid value < start LBA ! - # "seconf" 0x1200 => 0x17ff : size is increasing ! + assert '0x00000800 0x00000a00 "first"' in output + assert '0x00001000 0x00001200 "second"' in output
@pytest.mark.boardspec('sandbox') @pytest.mark.buildconfigspec('cmd_gpt') @@ -149,12 +145,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 10/18/2017 07:11 AM, Patrick Delaunay 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 rename or swap.
All the test/py changes in this series, Acked-by: Stephen Warren swarren@nvidia.com
The series, Tested-by: Stephen Warren swarren@nvidia.com
If you're looking for more things to change(!), perhaps change the sgdisk commands so that the partitions are big enough that 'gpt read host 0' says something other than 'size 0MiB' for the partitions; when I saw that, I was initially rather confused until I realized that the partitions were less than 1MiB!
Hi,
-----Original Message----- From: Stephen Warren [mailto:swarren@wwwdotorg.org]
On 10/18/2017 07:11 AM, Patrick Delaunay 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 rename or swap.
All the test/py changes in this series, Acked-by: Stephen Warren swarren@nvidia.com
The series, Tested-by: Stephen Warren swarren@nvidia.com
If you're looking for more things to change(!), perhaps change the sgdisk commands so that the partitions are big enough that 'gpt read host 0' says something other than 'size 0MiB' for the partitions; when I saw that, I was initially rather confused until I realized that the partitions were less than 1MiB!
I don't really looking... but I agree: size 0MiB is confusing, I had the same reaction the first time.
So I will propose something when this serie will be accepted, just update the test to have 2 partitions with different size and > 1MiB.
Regards Patrick
On Wed, Oct 18, 2017 at 03:11:08PM +0200, Patrick Delaunay 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 rename or swap.
Signed-off-by: Patrick Delaunay patrick.delaunay@st.com Acked-by: Stephen Warren swarren@nvidia.com Tested-by: Stephen Warren swarren@nvidia.com
Applied to u-boot/master, thanks!
participants (4)
-
Patrick DELAUNAY -
Patrick Delaunay
-
Stephen Warren -
Tom Rini