Code review comment for lp:~lool/linaro-image-tools/4-mib-align

On Thu, Feb 10, 2011, Guilherme Salgado wrote:
> > * qemu 4G maverick image, this wouldn't boot at first, I had to replace u-boot.bin and MLO and then it booted -- not sure why, don't think this is a regression
> Can you test this using an image generated by trunk? I thought that
> would work, but maybe I'm missing something?

 Ok; will try

> > * 1G overo image file dd-ed to a MMC, this booted and was aligned, but CHS was incorrect from Linux (didn't appear at 63/255 anymore)
>
> Is there any chance this wrong geometry would cause problems to the old
> OMAP3 roms that rely on specific values for CHS? Any way we could test
> it?

 There is; there is no good record of the actual limitation, so we're
 trying our best to do 63/255, but I don't know if it's enough as it is
 or not :-/

 I guess it might be that we need to have the boot part start at the
 beginning of a cylinder for x-loader.

 I have a C3 or C4 beagle I could test on; beagle xM worked so I'm not
 too worried -- worst case we use a different scheme for xm and for
 oldomap3?

> Neither relative nor absolute sizes can be specified for each of the
> partitions, right? Do we really care about that?

 Well, I think it would be helpful some day... but later :-)

> > - # This will create a partition of the given type, containing 9
> > - # cylinders (74027520 bytes, ~70 MiB), followed by a Linux-type
> > - # partition containing the rest of the free space.
> > - return ',9,%s,*\n,,,-' % partition_type
> > + # This will create a boot partition of type partition_type at offset
> > + # (in sectors) BOOT_PART_START_S and of size BOOT_PART_SIZE_S followed
> > + # by a root partition of the default type (Linux) at offset (in
> > + # sectors) ROOT_PART_START_S filling the remaining space
> > + # XXX we should honor the specified image size by specifying a
> > + # corresponding root partition size
>
> You mean we're no longer using the size as the upper limit for the image
> size or that the image size is not exactly what was requested? If the
> former, I think we need a bug to track it, but if it's the latter, do we
> really care about it?

 I don't think there's any regression, but I don't think we're enforcing
 the cylinder size properly; ideally, the sfdisk commands would include
 a length for the rootfs rather than "use all space".

> > - ',1,0xDA', 5, 63, '', tempfile, as_root=False,
> > + ',1,0xDA', 255, 63, '', tempfile, as_root=False,
>
> Why did you change the two tests above?

 I wanted to use 255/63 consistently

> > - sfdisk_commands, 5, 63, '', tempfile, as_root=False,
> > + sfdisk_commands, 255, 63, '', tempfile, as_root=False,
> I'd be interested in knowing what motivated this change as well.

 (same)

> > + # boot part at +8 MiB, root part at +16 MiB
> > + tempfile = self._create_qemu_img_with_partitions('16384,15746,0x0C,*\n32768,,,-')
> I've seen the above string hard-coded in way too many tests now so I
> think it'd make sense to make a constant out of it and use it here. That
> way you can even get rid of the comment above it.

 ack

--
Loïc Minier