Merge lp://qastaging/~mabac/linaro-image-tools/image-support-for-android into lp://qastaging/linaro-image-tools/11.11

Diff looks good to me, but I'm not sure about the -C stuff which we pass to sfdisk; I wonder whether we need the whole CHS complexity in multiple places, I'm not sure what it entails.

On Tue, May 31, 2011 at 11:59 AM, Loïc Minier <email address hidden> wrote:
> Diff looks good to me, but I'm not sure about the -C stuff which we pass to sfdisk; I wonder whether we need the whole CHS complexity in multiple places, I'm not sure what it entails.

If it possibly is not needed that might be worth investigating. To me
calculating stuff for block devices is scary and the less of it there
is the better I feel. I wouldn't know where to begin though, don't
have the history of why we do these calculations.

On Tue, May 31, 2011, Mattias Backman wrote:
> If it possibly is not needed that might be worth investigating. To me
> calculating stuff for block devices is scary and the less of it there
> is the better I feel. I wouldn't know where to begin though, don't
> have the history of why we do these calculations.

 In the early versions, we were trying to generate sizes which were
 aligned on CHS boundaries; there was also the desire to pass the right
 number of cylinders to sfdisk. Nowadays, I think only QEMU cares about
 the size of the image (I think it prefers if it's a power of 2, but not
 sure how picky it is) and we just try to align the start of partitions
 on power of 2, but that doesn't matter much.

 Perhaps now is the good time to break things so that we have the time
 to understand the new issues and fix them :-)

--
Loïc Minier

On Tue, May 31, 2011 at 3:12 PM, Loïc Minier <email address hidden> wrote:
> On Tue, May 31, 2011, Mattias Backman wrote:
>> If it possibly is not needed that might be worth investigating. To me
>> calculating stuff for block devices is scary and the less of it there
>> is the better I feel. I wouldn't know where to begin though, don't
>> have the history of why we do these calculations.
>
>  In the early versions, we were trying to generate sizes which were
>  aligned on CHS boundaries; there was also the desire to pass the right
>  number of cylinders to sfdisk.  Nowadays, I think only QEMU cares about
>  the size of the image (I think it prefers if it's a power of 2, but not
>  sure how picky it is) and we just try to align the start of partitions
>  on power of 2, but that doesn't matter much.
>
>  Perhaps now is the good time to break things so that we have the time
>  to understand the new issues and fix them  :-)

That sounds like fun. :) I've recorded that in lp:791334 so we
remember to break stuff after the long weekend.

Cheers, Mattias

Hi Mattias,

This looks good; I have only one suggestion below.

On Tue, 2011-05-31 at 08:04 +0000, Mattias Backman wrote:
> === modified file 'linaro_image_tools/media_create/partitions.py'
> --- linaro_image_tools/media_create/partitions.py 2011-05-31 08:04:29 +0000
> +++ linaro_image_tools/media_create/partitions.py 2011-05-31 08:04:29 +0000
[...]
> @@ -195,6 +212,21 @@
> return boot_device, root_device
>
>
> +def get_android_loopback_devices(image_file):
> + """Return the loopback devices for the given image file.
> +
> + Assumes a particular order of devices in the file.
> + Register the loopback devices as well.
> + """
> + devices = []
> + device_info = calculate_android_partition_size_and_offset(image_file)
> + for device_offset, device_size in device_info:
> + devices.append(register_loopback(image_file, device_offset,
> + device_size))
> +
> + return devices

> +
> +
> def register_loopback(image_file, offset, size):
> """Register a loopback device with an atexit handler to de-register it."""
> def undo(device):
> @@ -252,6 +284,33 @@
> "Couldn't find root partition on %s" % image_file)
> return vfat_size, vfat_offset, linux_size, linux_offset
>
> +
> +def calculate_android_partition_size_and_offset(image_file):
> + """Return the size and offset of the android partitions.
> +
> + Both the size and offset are in bytes.
> +
> + :param image_file: A string containing the path to the image_file.
> + :return: A list of (offset, size) pairs.
> + """
> + # Here we can use parted.Device to read the partitions because we're
> + # reading from a regular file rather than a block device. If it was a
> + # block device we'd need root rights.
> + disk = Disk(Device(image_file))
> + partition_info = []
> + for partition in disk.partitions:
> + geometry = partition.geometry
> + partition_info.append((geometry.start * SECTOR_SIZE,
> + geometry.length * SECTOR_SIZE))
> + # NB: don't use vfat_partition.nextPartition() as that might return
> + # a partition of type PARTITION_FREESPACE; it's much easier to
> + # iterate disk.partitions which only returns
> + # parted.PARTITION_NORMAL partitions
> +
> + assert len(partition_info) == 6
> + return partition_info

I think both calculate_android_partition_size_and_offset() and
get_android_loopback_devices() could do with some basic tests. Something
as simple as the existing test_calculate_partition_size_and_offset() and
test_get_boot_and_root_loopback_devices() would be enough, I think.
Would you be willing to give that a try? I expect it to be quite
straightforward.

--
Guilherme Salgado <https://launchpad.net/~salgado>

> I think both calculate_android_partition_size_and_offset() and
> get_android_loopback_devices() could do with some basic tests. Something
> as simple as the existing test_calculate_partition_size_and_offset() and
> test_get_boot_and_root_loopback_devices() would be enough, I think.
> Would you be willing to give that a try? I expect it to be quite
> straightforward.

Thanks, Guilherme you're right. Of course I'll add the tests, will
look at it after the extended weekend. :)

>
> --
> Guilherme Salgado <https://launchpad.net/~salgado>
>
> https://code.launchpad.net/~mabac/linaro-image-tools/image-support-for-android/+merge/62950
> You are the owner of lp:~mabac/linaro-image-tools/image-support-for-android.
>

Oh it actually does boot. At least when I specify the exact image size to --image_size like this for a 2GB SD card:

./linaro-android-media-create --boot boot.tar.bz2 --system system.tar.bz2 --userdata userdata.tar.bz2 --dev panda --image_file image_to_test_exact_size.img --image_size 2002780160

That size is what I get when I dd from a 2GB Android SD card that was written using --mmc.

If anyone can create an image like that, dd it and see if the android image boots, I'd appreciate it very much. Possibly using any other size card (I have only 2GB and 16GB atm).

Thanks,

Mattias

> I think both calculate_android_partition_size_and_offset() and
> get_android_loopback_devices() could do with some basic tests. Something
> as simple as the existing test_calculate_partition_size_and_offset() and
> test_get_boot_and_root_loopback_devices() would be enough, I think.
> Would you be willing to give that a try? I expect it to be quite
> straightforward.

There we go, one test each for those functions added. Let me know what you think.

Hi Mattias,

They look good to me; I've just a couple suggestions.

I'd use zip(seq1, seq2) instead of map(None, ...) as it feels more natural.

Also, it'd be nice to have a class variable with the partition sizes as a function of the sector numbers used in _create_android_tmpfile(), e.g:

  sizes_and_offsets = [
    (63 * 512, 270272 * 512),
    (270336 * 512, ...),
    ...]

That way if we ever need to change them in _create_android_tmpfile() we can easily do a search-and-replace to change them here as well. And by having this as a class variable we could use it in test_get_android_loopback_devices() as well, to construct the list of expected commands:

  expected = [
    '%s losetup -f --show %s --offset %s --sizelimit %s' % (sudo_args, tmpfile, size, offset)
    for (size, offset) in sizes_and_offsets]

> Hi Mattias,
>
> They look good to me; I've just a couple suggestions.
>
> I'd use zip(seq1, seq2) instead of map(None, ...) as it feels more natural.

I did consider zip, but it will only iterate over the number of items in the shortest list if the lists are not of equal length. So if the list of expected offsets and sizes would miss the values for the last partition the test would pass using zip. I guess that could happen by forgetting to add data for a new partition or just accidentally removing the last pair.

With the admittedly uglier map(None, ...) the test will fail like this:
AssertionError: Match failed. Matchee: "None"
Matcher: Equals((1212170240L, 935313408L))
Difference: (1212170240L, 935313408L) != None

Perhaps not very likely to be an issue but at least in theory the test would be a little more robust, I think.

> Also, it'd be nice to have a class variable with the partition sizes as a
> function of the sector numbers used in _create_android_tmpfile(), e.g:
>
> sizes_and_offsets = [
> (63 * 512, 270272 * 512),
> (270336 * 512, ...),
> ...]
>
> That way if we ever need to change them in _create_android_tmpfile() we can
> easily do a search-and-replace to change them here as well. And by having this
> as a class variable we could use it in test_get_android_loopback_devices() as
> well, to construct the list of expected commands:
>
> expected = [
> '%s losetup -f --show %s --offset %s --sizelimit %s' % (sudo_args,
> tmpfile, size, offset)
> for (size, offset) in sizes_and_offsets]

You're right that it made the tests a lot less cluttered and probably easier to maintain. I might do something similar for the non android functions too.

193 + self.sector_size = 512
194 + self.android_image_size = 2 * 1024**3 # rounded up from ~1.13GiB
195 + self.android_offsets_and_sizes = [
196 + (63 * self.sector_size, 270272 * self.sector_size),
197 + (270336 * self.sector_size, 524288 * self.sector_size),
198 + (794624 * self.sector_size, 524288 * self.sector_size),
199 + (1318912 * self.sector_size, (self.android_image_size -
200 + 1318912 * self.sector_size)),
201 + ((1318912 + 32) * self.sector_size, ((1048576 - 32) *
202 + self.sector_size)),
203 + ((2367488 + 32) * self.sector_size,
204 + self.android_image_size - (2367488 + 32) * self.sector_size)
205 + ]

The reason for adding and substracting 32 here is that it seems that the extended partition uses that much space. That space gets stolen from the first partition even though sizes and offsets are explicitly requested by the sfdisk commands.

On Fri, 2011-06-10 at 08:31 +0000, Mattias Backman wrote:
> > Hi Mattias,
> >
> > They look good to me; I've just a couple suggestions.
> >
> > I'd use zip(seq1, seq2) instead of map(None, ...) as it feels more natural.
>
> I did consider zip, but it will only iterate over the number of items
> in the shortest list if the lists are not of equal length. So if the
> list of expected offsets and sizes would miss the values for the last
> partition the test would pass using zip. I guess that could happen by
> forgetting to add data for a new partition or just accidentally
> removing the last pair.

Oh, good catch. It'd be nice to have a comment stating why we've used
map() so that people don't inadvertently change it to use zip().

>
> With the admittedly uglier map(None, ...) the test will fail like
> this:
> AssertionError: Match failed. Matchee: "None"
> Matcher: Equals((1212170240L, 935313408L))
> Difference: (1212170240L, 935313408L) != None
>
> Perhaps not very likely to be an issue but at least in theory the test
> would be a little more robust, I think.

Yeah, if there's any reason to use map() instead of zip() then by all
means do it. :)

>
> > Also, it'd be nice to have a class variable with the partition sizes as a
> > function of the sector numbers used in _create_android_tmpfile(), e.g:
> >
> > sizes_and_offsets = [
> > (63 * 512, 270272 * 512),
> > (270336 * 512, ...),
> > ...]
> >
> > That way if we ever need to change them in _create_android_tmpfile() we can
> > easily do a search-and-replace to change them here as well. And by having this
> > as a class variable we could use it in test_get_android_loopback_devices() as
> > well, to construct the list of expected commands:
> >
> > expected = [
> > '%s losetup -f --show %s --offset %s --sizelimit %s' % (sudo_args,
> > tmpfile, size, offset)
> > for (size, offset) in sizes_and_offsets]
>
> You're right that it made the tests a lot less cluttered and probably easier to maintain. I might do something similar for the non android functions too.

Cool, that's highly appreciated!

On Fri, 2011-06-10 at 10:56 +0000, Mattias Backman wrote:
> 193 + self.sector_size = 512
> 194 + self.android_image_size = 2 * 1024**3 # rounded up from ~1.13GiB

I'm confused by the 1.13GiB. I thought 1024**3 wass 1GiB?

> 195 + self.android_offsets_and_sizes = [
> 196 + (63 * self.sector_size, 270272 * self.sector_size),
> 197 + (270336 * self.sector_size, 524288 * self.sector_size),
> 198 + (794624 * self.sector_size, 524288 * self.sector_size),
> 199 + (1318912 * self.sector_size, (self.android_image_size -
> 200 + 1318912 * self.sector_size)),
> 201 + ((1318912 + 32) * self.sector_size, ((1048576 - 32) *
> 202 + self.sector_size)),
> 203 + ((2367488 + 32) * self.sector_size,
> 204 + self.android_image_size - (2367488 + 32) * self.sector_size)
> 205 + ]
>
> The reason for adding and substracting 32 here is that it seems that
> the extended partition uses that much space. That space gets stolen
> from the first partition even though sizes and offsets are explicitly
> requested by the sfdisk commands.

You must've had some fun to figure that out, eh? :P

It'd be nice to have this as a comment, I think.

On Fri, Jun 10, 2011 at 3:25 PM, Guilherme Salgado
<email address hidden> wrote:
> On Fri, 2011-06-10 at 10:56 +0000, Mattias Backman wrote:
>> 193   + self.sector_size = 512
>> 194   + self.android_image_size = 2 * 1024**3 # rounded up from ~1.13GiB
>
> I'm confused by the 1.13GiB.  I thought 1024**3 wass 1GiB?

Oh, yes. The sfdisk command (stolen from the production code) creates
partitions that would consume at least 1.13 GiB. So I figured I'd
round that up to 2 GiB since that would be more like an actual size sd
card than fractions of GiBs. I'm still not sure if I should clarify
the comment or change the partition sizes so we don't create a 2 GiB
tempfile.

>
>> 195   + self.android_offsets_and_sizes = [
>> 196   + (63 * self.sector_size, 270272 * self.sector_size),
>> 197   + (270336 * self.sector_size, 524288 * self.sector_size),
>> 198   + (794624 * self.sector_size, 524288 * self.sector_size),
>> 199   + (1318912 * self.sector_size, (self.android_image_size -
>> 200   + 1318912 * self.sector_size)),
>> 201   + ((1318912 + 32) * self.sector_size, ((1048576 - 32) *
>> 202   + self.sector_size)),
>> 203   + ((2367488 + 32) * self.sector_size,
>> 204   + self.android_image_size - (2367488 + 32) * self.sector_size)
>> 205   + ]
>>
>> The reason for adding and substracting 32 here is that it seems that
>> the extended partition uses that much space. That space gets stolen
>> from the first partition even though sizes and offsets are explicitly
>> requested by the sfdisk commands.
>
> You must've had some fun to figure that out, eh? :P

Well there are people who consider pain to be fun, but I'm not one of them. ;)

> It'd be nice to have this as a comment, I think.

True, I'll add a comment about this as well as the map/zip decision.

On Fri, 2011-06-10 at 13:46 +0000, Mattias Backman wrote:
> On Fri, Jun 10, 2011 at 3:25 PM, Guilherme Salgado
> <email address hidden> wrote:
> > On Fri, 2011-06-10 at 10:56 +0000, Mattias Backman wrote:
> >> 193 + self.sector_size = 512
> >> 194 + self.android_image_size = 2 * 1024**3 # rounded up from ~1.13GiB
> >
> > I'm confused by the 1.13GiB. I thought 1024**3 wass 1GiB?
>
> Oh, yes. The sfdisk command (stolen from the production code) creates
> partitions that would consume at least 1.13 GiB. So I figured I'd
> round that up to 2 GiB since that would be more like an actual size sd
> card than fractions of GiBs. I'm still not sure if I should clarify
> the comment or change the partition sizes so we don't create a 2 GiB
> tempfile.

Yeah, it'd be nice to avoid creating a file that big if possible. Do you
have any idea how much the creation of a 2GiB file adds up to the
runtime of the test suite?

On Mon, Jun 13, 2011 at 3:21 PM, Guilherme Salgado
<email address hidden> wrote:
> On Fri, 2011-06-10 at 13:46 +0000, Mattias Backman wrote:
>> On Fri, Jun 10, 2011 at 3:25 PM, Guilherme Salgado
>> <email address hidden> wrote:
>> > On Fri, 2011-06-10 at 10:56 +0000, Mattias Backman wrote:
>> >> 193   + self.sector_size = 512
>> >> 194   + self.android_image_size = 2 * 1024**3 # rounded up from ~1.13GiB
>> >
>> > I'm confused by the 1.13GiB.  I thought 1024**3 wass 1GiB?
>>
>> Oh, yes. The sfdisk command (stolen from the production code) creates
>> partitions that would consume at least 1.13 GiB. So I figured I'd
>> round that up to 2 GiB since that would be more like an actual size sd
>> card than fractions of GiBs. I'm still not sure if I should clarify
>> the comment or change the partition sizes so we don't create a 2 GiB
>> tempfile.
>
> Yeah, it'd be nice to avoid creating a file that big if possible. Do you
> have any idea how much the creation of a 2GiB file adds up to the
> runtime of the test suite?

I'll tweak the partitions for the test file. The test doesn't take any
more time when creating a huge file than creating a small one. All it
does is to write nothing to the file since count=0. So it just touches
the file I guess, which seems instantaneous and then creates
partitions with sfdisk.

On Tue, Jun 14, 2011, Mattias Backman wrote:
> I'll tweak the partitions for the test file. The test doesn't take any
> more time when creating a huge file than creating a small one. All it
> does is to write nothing to the file since count=0. So it just touches
> the file I guess, which seems instantaneous and then creates
> partitions with sfdisk.

 Perhaps this only holds true if the filesystem supports sparse files?
 It's not that common outside of ext3/4; for instance I use ecryptfs (a
 layered filesystem) on top of ext4 and I don't have sparse files. It
 would be painful to create a real 2 GiB file (and crypt it! :) each
 time I run the testsuite

--
Loïc Minier

> On Tue, Jun 14, 2011, Mattias Backman wrote:
> > I'll tweak the partitions for the test file. The test doesn't take any
> > more time when creating a huge file than creating a small one. All it
> > does is to write nothing to the file since count=0. So it just touches
> > the file I guess, which seems instantaneous and then creates
> > partitions with sfdisk.
>
> Perhaps this only holds true if the filesystem supports sparse files?
> It's not that common outside of ext3/4; for instance I use ecryptfs (a
> layered filesystem) on top of ext4 and I don't have sparse files. It
> would be painful to create a real 2 GiB file (and crypt it! :) each
> time I run the testsuite

That does sound pretty painful. :) I have reduced it to 256 MiB, please let me know if that still hurts too bad.

The default size image worked fine now when I wrote it to a 16 GiB SD. So I'll test some more on the 2 GiB card and see what happens.

> Oh it actually does boot. At least when I specify the exact image size to
> --image_size like this for a 2GB SD card:
>
> ./linaro-android-media-create --boot boot.tar.bz2 --system system.tar.bz2
> --userdata userdata.tar.bz2 --dev panda --image_file
> image_to_test_exact_size.img --image_size 2002780160
>
> That size is what I get when I dd from a 2GB Android SD card that was written
> using --mmc.

Now this mystery is solved. My Sandisk card marketed as "2GB" is of course not 2GiB, it's only 1.83GiB so the default 2G image_size does not fit. Trivial user error but I just didn't think about it.