Merge lp:~lool/linaro-image-tools/part-size-rework into lp:linaro-image-tools/11.11

Proposed by Loïc Minier
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
Reviewer Review Type Date Requested Status
Guilherme Salgado (community) Approve
Review via email: mp+49161@code.launchpad.net

Description of the change

The relatively simple fixes prepare for 4 MiB aligned partitions

To post a comment you must log in.
Revision history for this message
Guilherme Salgado (salgado) wrote :
Download full text (4.1 KiB)

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:
> differences between files attachment (review-diff.txt)
> === modified file 'linaro_media_create/partitions.py'
> --- linaro_media_create/partitions.py 2011-01-28 19:50:48 +0000
> +++ linaro_media_create/partitions.py 2011-02-10 01:28:53 +0000
> @@ -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_block_device:
> image_size_in_bytes = convert_size_to_bytes(image_size)
> 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_size_in_bytes)],

This looks weird to me. Do you find that makes it more readable or was
it just accidental?

> stdout=open('/dev/null', 'w'))
> proc.wait()
>
> @@ -173,19 +175,29 @@
> # block device we'd need root rights.
> disk = Disk(Device(image_file))
> 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.getFlagsAsString():
> 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.nextPartition() as that might return
> + # a partition of type PARTITION_FREESPACE; it's much easier to

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
> + # parted.PARTITION_NORMAL partitions
> + 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.

>
> assert vfat_partition is not None, (
> "Couldn't find boot partition on %s" % image_file)
> - linux_partition = vfat_partition.nextPartition()
> - geometry = linux_partition.geometry
> - l...

Read more...

review: Approve
Revision history for this message
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_size_in_bytes)],
>
> 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.getFlagsAsString():
> > 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.nextPartition() as that might return
> > + # a partition of type PARTITION_FREESPACE; it's much easier to
>
> 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.PARTITION_NORMAL partitions
> > + 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

Revision history for this message
Guilherme Salgado (salgado) wrote :
Download full text (3.2 KiB)

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.getFlagsAsString():
> > > 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.nextPartition() as that might return
> > > + # a partition of type PARTITION_FREESPACE; it's much easier to
> >
> > 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.PARTITION_NORMAL partitions
> > > + 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_qemu_img_with_partitions() and check that the
offset of th...

Read more...

Revision history for this message
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.

Revision history for this message
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"

review: Approve
296. By Loïc Minier

Add comment to test_partition_numbering().

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
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],

Subscribers

People subscribed via source and target branches