Merge lp:~lool/linaro-image-tools/part-size-rework into lp:linaro-image-tools/11.11
- part-size-rework
- Merge into trunk
Status: | Merged |
---|---|
Merged at revision: | 287 |
Proposed branch: | lp:~lool/linaro-image-tools/part-size-rework |
Merge into: | lp:linaro-image-tools/11.11 |
Diff against target: |
90 lines (+31/-7) 2 files modified
linaro_media_create/partitions.py (+20/-6) linaro_media_create/tests/test_media_create.py (+11/-1) |
To merge this branch: | bzr merge lp:~lool/linaro-image-tools/part-size-rework |
Related bugs: |
Reviewer | Review Type | Date Requested | Status |
---|---|---|---|
Guilherme Salgado (community) | Approve | ||
Review via email: mp+49161@code.launchpad.net |
Commit message
Description of the change
The relatively simple fixes prepare for 4 MiB aligned partitions
Loïc Minier (lool) wrote : | # |
On Thu, Feb 10, 2011, Guilherme Salgado wrote:
> > - image_size_in_bytes = cylinders * CYLINDER_SIZE
> > proc = cmd_runner.run(
> > - ['qemu-img', 'create', '-f', 'raw', media.path, image_size],
> > + [
> > + 'qemu-img', 'create', '-f', 'raw', media.path,
> > + str(image_
>
> This looks weird to me. Do you find that makes it more readable or was
> it just accidental?
I had to wrap it because it was above 80 chars wide, but my indent is a
bit weird indeed
> > + linux_partition = None
> > for partition in disk.partitions:
> > + assert partition.type == PARTITION_NORMAL
>
> Care to add a comment to this assertion? Something like "We expect
> disk.partitions to include only normal partitions", maybe?
Hmm ok; it's probably best to cover it there rather than in the rootfs
block
> > if 'boot' in partition.
> > geometry = partition.geometry
> > vfat_offset = geometry.start * 512
> > vfat_size = geometry.length * 512
> > vfat_partition = partition
> > + elif vfat_partition is not None:
> > + # next partition after boot partition is the root partition
> > + # NB: don't use vfat_partition.
> > + # a partition of type PARTITION_
>
> How did you trigger that? I guess what I'm really asking is how did you
> end up with freespace between the boot and root partitions?
I triggered this with the 4 MiB alignment branch, but realizing that
this was affecting the existing code, I decided to fix it here first;
hope that's ok. This is being exercized by the testsuite in the 4 MiB
alignment branch since the testsuite uses 4 MiB aligned partitions too
and hence there is some freespace between partitions. The losetup test
returned different (incorrect) values before this change.
> > + # iterate disk.partitions which only returns
> > + # parted.
> > + geometry = partition.geometry
> > + linux_offset = geometry.start * 512
> > + linux_size = geometry.length * 512
> > + linux_partition = partition
>
> This is not going to work if there's more than one partition after the
> boot partition, as the last one will be used instead of the one right
> after the boot partition. I don't think we currently have more than one
> partition after the boot one for any board, but maybe we could have in
> the future?
Oh right, I thought of this and then failed to implement a test for
whether rootfs was already set; i think I want to change:
elif vfat_partition is not None:
to:
elif vfat_partition is not None and linux_partition is None:
would that be ok?
> It'd be good to have a test showing that the root partition is always
> the one right next to the boot one.
Hmm, I'll have to think about this one
--
Loïc Minier
Guilherme Salgado (salgado) wrote : | # |
On Thu, 2011-02-10 at 13:56 +0000, Loïc Minier wrote:
> > > + linux_partition = None
> > > for partition in disk.partitions:
> > > + assert partition.type == PARTITION_NORMAL
> >
> > Care to add a comment to this assertion? Something like "We expect
> > disk.partitions to include only normal partitions", maybe?
>
> Hmm ok; it's probably best to cover it there rather than in the rootfs
> block
Agreed, but what I meant is to add a failure message to the assert
rather than a comment. It's good to always have failure messages in your
asserts.
>
> > > if 'boot' in partition.
> > > geometry = partition.geometry
> > > vfat_offset = geometry.start * 512
> > > vfat_size = geometry.length * 512
> > > vfat_partition = partition
> > > + elif vfat_partition is not None:
> > > + # next partition after boot partition is the root partition
> > > + # NB: don't use vfat_partition.
> > > + # a partition of type PARTITION_
> >
> > How did you trigger that? I guess what I'm really asking is how did you
> > end up with freespace between the boot and root partitions?
>
> I triggered this with the 4 MiB alignment branch, but realizing that
> this was affecting the existing code, I decided to fix it here first;
> hope that's ok. This is being exercized by the testsuite in the 4 MiB
> alignment branch since the testsuite uses 4 MiB aligned partitions too
> and hence there is some freespace between partitions. The losetup test
> returned different (incorrect) values before this change.
Yeah, I guessed that when reading your 4MiB alignment branch. Thanks
for the clarification, and I think it's fine to fix it here.
>
> > > + # iterate disk.partitions which only returns
> > > + # parted.
> > > + geometry = partition.geometry
> > > + linux_offset = geometry.start * 512
> > > + linux_size = geometry.length * 512
> > > + linux_partition = partition
> >
> > This is not going to work if there's more than one partition after the
> > boot partition, as the last one will be used instead of the one right
> > after the boot partition. I don't think we currently have more than one
> > partition after the boot one for any board, but maybe we could have in
> > the future?
>
> Oh right, I thought of this and then failed to implement a test for
> whether rootfs was already set; i think I want to change:
> elif vfat_partition is not None:
> to:
> elif vfat_partition is not None and linux_partition is None:
> would that be ok?
I think a break after setting linux_partition would be better, as it
makes it clear we're done once we set linux_partition.
>
> > It'd be good to have a test showing that the root partition is always
> > the one right next to the boot one.
>
> Hmm, I'll have to think about this one
It should be easy, I think; just pass a sfdisk command that creates 3
partitions to self._create_
offset of th...
Loïc Minier (lool) wrote : | # |
Pushed suggests fixes and tests
- 289. By Loïc Minier
-
Fix weird line-wrapping of mine.
- 290. By Loïc Minier
-
Add some helpful message in the "normal partition" assert.
- 291. By Loïc Minier
-
Add an explicit break when we find the linux partition as to not use a
partition found afterwards. - 292. By Loïc Minier
-
Add some tests to make sure rootfs is after bootfs and that partitions don't
overlap. - 293. By Loïc Minier
-
Remove debug print.
- 294. By Loïc Minier
-
Drop tests which are probably useless after discussion -- let's not bloat the
testsuite. - 295. By Loïc Minier
-
Add test to make sure first partition after vfat is selected as the linux
partition.
Guilherme Salgado (salgado) wrote : | # |
r295 looks nice; I'd just add a comment on the test explaining that we expect the code to use the partition right next to the vfat one as the root partition and why we expect linux_offset to be "5 * 63 * 512"
- 296. By Loïc Minier
-
Add comment to test_partition_
numbering( ).
Preview Diff
1 | === modified file 'linaro_media_create/partitions.py' |
2 | --- linaro_media_create/partitions.py 2011-01-28 19:50:48 +0000 |
3 | +++ linaro_media_create/partitions.py 2011-02-10 18:57:06 +0000 |
4 | @@ -27,6 +27,7 @@ |
5 | from parted import ( |
6 | Device, |
7 | Disk, |
8 | + PARTITION_NORMAL, |
9 | ) |
10 | |
11 | from linaro_media_create import cmd_runner |
12 | @@ -60,9 +61,9 @@ |
13 | if not media.is_block_device: |
14 | image_size_in_bytes = convert_size_to_bytes(image_size) |
15 | cylinders = image_size_in_bytes / CYLINDER_SIZE |
16 | - image_size_in_bytes = cylinders * CYLINDER_SIZE |
17 | proc = cmd_runner.run( |
18 | - ['qemu-img', 'create', '-f', 'raw', media.path, image_size], |
19 | + ['qemu-img', 'create', '-f', 'raw', media.path, |
20 | + str(image_size_in_bytes)], |
21 | stdout=open('/dev/null', 'w')) |
22 | proc.wait() |
23 | |
24 | @@ -173,19 +174,32 @@ |
25 | # block device we'd need root rights. |
26 | disk = Disk(Device(image_file)) |
27 | vfat_partition = None |
28 | + linux_partition = None |
29 | for partition in disk.partitions: |
30 | + assert partition.type == PARTITION_NORMAL, ( |
31 | + "Parted should only return normal partitions but got type %i" % |
32 | + partition.type) |
33 | if 'boot' in partition.getFlagsAsString(): |
34 | geometry = partition.geometry |
35 | vfat_offset = geometry.start * 512 |
36 | vfat_size = geometry.length * 512 |
37 | vfat_partition = partition |
38 | + elif vfat_partition is not None: |
39 | + # next partition after boot partition is the root partition |
40 | + # NB: don't use vfat_partition.nextPartition() as that might return |
41 | + # a partition of type PARTITION_FREESPACE; it's much easier to |
42 | + # iterate disk.partitions which only returns |
43 | + # parted.PARTITION_NORMAL partitions |
44 | + geometry = partition.geometry |
45 | + linux_offset = geometry.start * 512 |
46 | + linux_size = geometry.length * 512 |
47 | + linux_partition = partition |
48 | + break |
49 | |
50 | assert vfat_partition is not None, ( |
51 | "Couldn't find boot partition on %s" % image_file) |
52 | - linux_partition = vfat_partition.nextPartition() |
53 | - geometry = linux_partition.geometry |
54 | - linux_offset = geometry.start * 512 |
55 | - linux_size = geometry.length * 512 |
56 | + assert linux_partition is not None, ( |
57 | + "Couldn't find root partition on %s" % image_file) |
58 | return vfat_size, vfat_offset, linux_size, linux_offset |
59 | |
60 | |
61 | |
62 | === modified file 'linaro_media_create/tests/test_media_create.py' |
63 | --- linaro_media_create/tests/test_media_create.py 2011-02-10 16:58:11 +0000 |
64 | +++ linaro_media_create/tests/test_media_create.py 2011-02-10 18:57:06 +0000 |
65 | @@ -722,6 +722,16 @@ |
66 | [129024L, 32256L, 10321920L, 161280L], |
67 | [vfat_size, vfat_offset, linux_size, linux_offset]) |
68 | |
69 | + def test_partition_numbering(self): |
70 | + # another Linux partition after the boot/root parts |
71 | + tempfile = self._create_qemu_img_with_partitions( |
72 | + ',1,0x0C,*\n,1,,-\n,,,-') |
73 | + vfat_size, vfat_offset, linux_size, linux_offset = ( |
74 | + calculate_partition_size_and_offset(tempfile)) |
75 | + # check that the linux partition offset starts on second cylinder so |
76 | + # that it's the partition immediately following the vfat one |
77 | + self.assertEqual(linux_offset, 5 * 63 * 512) |
78 | + |
79 | def test_get_boot_and_root_partitions_for_media_beagle(self): |
80 | self.useFixture(MockSomethingFixture( |
81 | partitions, '_get_device_file_for_partition_number', |
82 | @@ -829,7 +839,7 @@ |
83 | 'root', 'ext3', True, True, True) |
84 | self.assertEqual( |
85 | # This is the call that would create the image file. |
86 | - [['qemu-img', 'create', '-f', 'raw', tempfile, '2G'], |
87 | + [['qemu-img', 'create', '-f', 'raw', tempfile, '2147483648'], |
88 | # This call would partition the image file. |
89 | ['sudo', 'sfdisk', '-D', '-H', '255', '-S', '63', '-C', '261', |
90 | tempfile], |
Hi Loïc,
Thanks for the fix and the improvements; I have some suggestions below
but I think this looks good.
review approve
On Thu, 2011-02-10 at 01:29 +0000, Loïc Minier wrote: media_create/ partitions. py' media_create/ partitions. py 2011-01-28 19:50:48 +0000 media_create/ partitions. py 2011-02-10 01:28:53 +0000 block_device: size_to_ bytes(image_ size) size_in_ bytes)] ,
> differences between files attachment (review-diff.txt)
> === modified file 'linaro_
> --- linaro_
> +++ linaro_
> @@ -27,6 +27,7 @@
> from parted import (
> Device,
> Disk,
> + PARTITION_NORMAL,
> )
>
> from linaro_media_create import cmd_runner
> @@ -60,9 +61,10 @@
> if not media.is_
> image_size_in_bytes = convert_
> cylinders = image_size_in_bytes / CYLINDER_SIZE
> - image_size_in_bytes = cylinders * CYLINDER_SIZE
> proc = cmd_runner.run(
> - ['qemu-img', 'create', '-f', 'raw', media.path, image_size],
> + [
> + 'qemu-img', 'create', '-f', 'raw', media.path,
> + str(image_
This looks weird to me. Do you find that makes it more readable or was
it just accidental?
> stdout= open('/ dev/null' , 'w')) image_file) )
> proc.wait()
>
> @@ -173,19 +175,29 @@
> # block device we'd need root rights.
> disk = Disk(Device(
> vfat_partition = None
> + linux_partition = None
> for partition in disk.partitions:
> + assert partition.type == PARTITION_NORMAL
Care to add a comment to this assertion? Something like "We expect
disk.partitions to include only normal partitions", maybe?
> if 'boot' in partition. getFlagsAsStrin g(): nextPartition( ) as that might return FREESPACE; it's much easier to
> geometry = partition.geometry
> vfat_offset = geometry.start * 512
> vfat_size = geometry.length * 512
> vfat_partition = partition
> + elif vfat_partition is not None:
> + # next partition after boot partition is the root partition
> + # NB: don't use vfat_partition.
> + # a partition of type PARTITION_
How did you trigger that? I guess what I'm really asking is how did you
end up with freespace between the boot and root partitions?
> + # iterate disk.partitions which only returns PARTITION_ NORMAL partitions
> + # parted.
> + geometry = partition.geometry
> + linux_offset = geometry.start * 512
> + linux_size = geometry.length * 512
> + linux_partition = partition
This is not going to work if there's more than one partition after the
boot partition, as the last one will be used instead of the one right
after the boot partition. I don't think we currently have more than one
partition after the boot one for any board, but maybe we could have in
the future?
It'd be good to have a test showing that the root partition is always
the one right next to the boot one.
> nextPartition( ) .geometry
> assert vfat_partition is not None, (
> "Couldn't find boot partition on %s" % image_file)
> - linux_partition = vfat_partition.
> - geometry = linux_partition
> - l...