Merge lp:~milo/linaro-image-tools/bug1004199 into lp:linaro-image-tools/11.11

[Milo Casagrande]

Hello,

the proposed branch should fix the image size problem.

Thanks to Michael suggestion, and after a brief chat with him on IRC, I reworked the convert_size_to_byte method to address the 1MiB multiple image size problem: I kept only one exit point for the method; added the final check on the size just before returning the value; added the comparable value at the beginning of the file as a constant named ROUND_IMAGE_TO; imported the math.floor function since the final check relies on that.

I added also a new method, _check_min_size, to ensure that we have at least 1MiB image size (this was the bit discussed on IRC).

I reworked and adapted a little bit the tests too. I added a new test for the exception that was not tested before; added a new one for the default size and fixed the other tests in order to address the changes in the function. One test has been removed since it was not really relevant anymore.

Cheers.

[Данило Шеган (danilo)]

Hi Milo,

Thanks for working on this fix. Overall, it looks great. A few
suggestions are inline, though.

У уто, 05. 06 2012. у 13:53 +0000, Milo Casagrande пише:

> the proposed branch should fix the image size problem.
>
> Thanks to Michael suggestion, and after a brief chat with him on IRC, I
> reworked the convert_size_to_byte method to address the 1MiB multiple
> image size problem: I kept only one exit point for the method; added
> the final check on the size just before returning the value; added the
> comparable value at the beginning of the file as a constant named
> ROUND_IMAGE_TO; imported the math.floor function since the final check
> relies on that.

With these changes, the name of the method might not make that much
sense anymore. It should be

 round_to_megs_ensure_larger_than_one_meg_and_convert_to_bytes :)

Perhaps use something that describes the nature of the method is now
more appropriate (i.e. "get_partition_size_in_bytes" or something
similar).

> I added also a new method, _check_min_size, to ensure that we have at least 1MiB image size (this was the bit discussed on IRC).

It would be good to add a few minimal tests for it as well :)

> I reworked and adapted a little bit the tests too. I added a new test
> for the exception that was not tested before; added a new one for the
> default size and fixed the other tests in order to address the changes
> in the function. One test has been removed since it was not really
> relevant anymore.

They look great, thanks.

> разлике међу датотекама прилог (review-diff.txt)
> === modified file 'linaro_image_tools/media_create/partitions.py'
...
> @@ -45,6 +45,9 @@
> # Max number of attempts to sleep (total sleep time in seconds =
> # 1+2+...+MAX_TTS)
> MAX_TTS = 10
> +# Image size should be a multiple of 1MiB, expressed in bytes. This is also
> +# the minimum image size possible.
> +ROUND_IMAGE_TO = 2 ** 20

I'd rather if we express this as two separate values, even if one is
defined through the other. Eg.

ROUND_IMAGE_TO = 2 ** 20
MIN_IMAGE_SIZE = ROUND_IMAGE_TO

> @@ -481,11 +484,12 @@
> def convert_size_to_bytes(size):
> """Convert a size string in Kbytes, Mbytes or Gbytes to bytes."""

It'd be nice to extend this to document the new features (min size,
rounding).

>...
> + # Guarantee that is a multiple of 1MiB, defined in ROUND_IMAGE_TO
> + real_size = _check_min_size(floor(real_size / ROUND_IMAGE_TO) *
> + ROUND_IMAGE_TO)

Better way to do the rounding is always to add half of ROUND_IMAGE_TO
and then simply round() it. This will ensure that you've got enough
space at all times.

Eg.

  round((real_size + ROUND_IMAGE_TO/2)/ROUND_IMAGE_TO) * ROUND_IMAGE_TO

will give you an image size which will be able to fit the requested
number of KBs or MBs (eg. asking for '1024.4K' will give you 2MB which
can really fit the requested number of kilobytes; it'd be even worse if
you ask for 1500K and get 1MB).

> return int(round(real_size))
>
>
> +def _check_min_size(size):
> + """Check that the image size is at least 1MiB (expressed in bytes)."""

Hard-coding a value here which is not really hard-coded doesn't make
much sense. ...

review: Needs Fixing

[Milo Casagrande (milo)]

Hello Danilo,

On Wed, Jun 6, 2012 at 9:55 AM, Данило Шеган <email address hidden> wrote:
>
> With these changes, the name of the method might not make that much
> sense anymore.  It should be
>
>  round_to_megs_ensure_larger_than_one_meg_and_convert_to_bytes :)
>
> Perhaps use something that describes the nature of the method is now
> more appropriate (i.e. "get_partition_size_in_bytes" or something
> similar).

I will use your suggestion for the method name. :-)

>> I added also a new method, _check_min_size, to ensure that we have at least 1MiB image size (this was the bit discussed on IRC).
>
> It would be good to add a few minimal tests for it as well :)

Added two new tests: one for a smaller value, one for a bigger one.
I added also a comment on _check_min_size, in order to avoid testing
passing size as a string: in the function "size" is a number. I used
"@param", is it the blessed way or is it better to use ":param"?

> I'd rather if we express this as two separate values, even if one is
> defined through the other. Eg.
>
> ROUND_IMAGE_TO = 2 ** 20
> MIN_IMAGE_SIZE = ROUND_IMAGE_TO

Fixed.

>> @@ -481,11 +484,12 @@
>>  def convert_size_to_bytes(size):
>>      """Convert a size string in Kbytes, Mbytes or Gbytes to bytes."""
>
> It'd be nice to extend this to document the new features (min size,
> rounding).

Added a small description of what is happening, another note on this
follows on the comment below.

>>...
>> +    # Guarantee that is a multiple of 1MiB, defined in ROUND_IMAGE_TO
>> +    real_size = _check_min_size(floor(real_size / ROUND_IMAGE_TO) *
>> +                                ROUND_IMAGE_TO)
>
> Better way to do the rounding is always to add half of ROUND_IMAGE_TO
> and then simply round() it.  This will ensure that you've got enough
> space at all times.
> Eg.
>
>  round((real_size + ROUND_IMAGE_TO/2)/ROUND_IMAGE_TO) * ROUND_IMAGE_TO
>
> will give you an image size which will be able to fit the requested
> number of KBs or MBs (eg. asking for '1024.4K' will give you 2MB which
> can really fit the requested number of kilobytes; it'd be even worse if
> you ask for 1500K and get 1MB).

Hmm... OK, got your point to have always enough space, although in
this case if we ask for '1M' we receive back a size of 2MiB, and
always a little bit more for all the other values. I hope it is not an
issue with space on users side if they want a better precision.

Changing this introduced a regression on another test, specifically
'test_setup_partitions_for_image_file' that tests 'setup_partitions':
image size in bytes here is used with 'dd'.

I changed the behavior of the converting functions, saying also that
the conversion in bytes is "deliberaltey inaccurate" (in the sense
that we add a small delta).
Fixed now, and also the tests and their values. :-)

>> +def _check_min_size(size):
>> +    """Check that the image size is at least 1MiB (expressed in bytes)."""
>
> Hard-coding a value here which is not really hard-coded doesn't make
> much sense.  Why not make it 'is at least MIN_IMAGE_SIZE bytes.'
> instead?

Fixed.

>> === modified file 'linaro_image_tools/media_create/tests/test_media_create.py'
>
> These are not of your doing, bu...

[Данило Шеган (danilo)]

Hi Milo,

Sorry for taking this long to respond: it got off my radar. Please feel
free to ping and poke me to be faster about finishing a review for you.

Thanks for all the fixes, it looks much better now. Just a few small
notes now.

У сре, 06. 06 2012. у 16:22 +0000, Milo Casagrande пише:

> Added two new tests: one for a smaller value, one for a bigger one.
> I added also a comment on _check_min_size, in order to avoid testing
> passing size as a string: in the function "size" is a number. I used
> "@param", is it the blessed way or is it better to use ":param"?

It's better to use :param since it's used in the rest of the code-base.

> Hmm... OK, got your point to have always enough space, although in
> this case if we ask for '1M' we receive back a size of 2MiB, and
> always a little bit more for all the other values. I hope it is not an
> issue with space on users side if they want a better precision.

Let's fix that as well :)

Would doing "- 0.5" (in bytes) before division by ROUND_TO_SIZE and
rounding help with that without breaking anything else? Will this
simplistic formula work for a case where we ask for 1.000001M (when we
do want 2M)?

To clarify the algorithm I proposed though, the "trick" of adding
ROUND_TO_SIZE/2 is a common approach to round up ("ceil"), and that's
all we are doing. So, the docstring should only mention rounding up,
and we should fix it up so that's all it does. Perhaps we can instead
do a math.ceil which I wanted to avoid for no particular reason other
than looking smarter than I am :)

> Changing this introduced a regression on another test, specifically
> 'test_setup_partitions_for_image_file' that tests 'setup_partitions':
> image size in bytes here is used with 'dd'.

Right, we might need to update a test or two with these changes.

> I changed the behavior of the converting functions, saying also that
> the conversion in bytes is "deliberaltey inaccurate" (in the sense
> that we add a small delta).

I'd rather we fix the problem and just describe it as "we round up and
in multiples of 1MB" or something.

Cheers,
Danilo

[Milo Casagrande (milo)]

Hello Danilo,

thanks for the review!
The final bits for the conversion should now be fixed.

We will use math.ceil, and return the correct rounded-up size for the image.
I added also a new test, based on your example, so that we can see the
difference and the correctness of asking 1M (that will return exactly
1MiB) compared to asking 1.0000001M (that will return 2MiB).

There might be also other changes, since I merged from trunk.
Ciao.

--
Milo Casagrande
Infrastructure Engineer
Linaro.org <www.linaro.org> │ Open source software for ARM SoCs

[Данило Шеган (danilo)]

Looks good, thanks for all the effort you expended on this bug :)

review: Approve

[Данило Шеган (danilo)]

Actually, we should simply use ceil, and get rid of the + 0.5 trick altogether.

I.e.

 real_size = _check_min_size(ceil(real_size / ROUND_IMAGE_TO) * ROUND_IMAGE_TO))

review: Needs Fixing

[lp:~milo/linaro-image-tools/bug1004199]

527. By Milo Casagrande on 2012-06-13

Removed round function.

[Milo Casagrande (milo)]

On Wed, Jun 13, 2012 at 3:33 PM, Данило Шеган <email address hidden> wrote:
>
> Actually, we should simply use ceil, and get rid of the + 0.5 trick altogether.
>
> I.e.
>
>  real_size = _check_min_size(ceil(real_size / ROUND_IMAGE_TO) * ROUND_IMAGE_TO))

With latest push I removed the "round" function, keeping only the
"int" conversion here, instead of doing it in the return statement,
sice we will execute _check_min_size anyway with MIN_IMAGE_SIZE that
is an int.

--
Milo Casagrande
Infrastructure Engineer
Linaro.org <www.linaro.org> │ Open source software for ARM SoCs

[Данило Шеган (danilo)]

Looks good, thanks!

review: Approve

=== modified file 'linaro_image_tools/media_create/partitions.py'
--- linaro_image_tools/media_create/partitions.py	2012-05-09 21:25:20 +0000
+++ linaro_image_tools/media_create/partitions.py	2012-06-13 13:45:20 +0000
@@ -17,15 +17,16 @@
 # You should have received a copy of the GNU General Public License
 # along with Linaro Image Tools.  If not, see <http://www.gnu.org/licenses/>.
 
+from contextlib import contextmanager
+from math import ceil
 import atexit
-from contextlib import contextmanager
+import dbus
 import glob
 import logging
 import re
 import subprocess
 import time
 
-import dbus
 from parted import (
     Device,
     Disk,
@@ -35,7 +36,6 @@
 
 from linaro_image_tools import cmd_runner
 
-
 HEADS = 128
 SECTORS = 32
 SECTOR_SIZE = 512  # bytes
@@ -45,13 +45,17 @@
 # Max number of attempts to sleep (total sleep time in seconds =
 # 1+2+...+MAX_TTS)
 MAX_TTS = 10
+# Image size should be a multiple of 1MiB, expressed in bytes. This is also
+# the minimum image size possible.
+ROUND_IMAGE_TO = 2 ** 20
+MIN_IMAGE_SIZE = ROUND_IMAGE_TO
 
 
 def setup_android_partitions(board_config, media, image_size, bootfs_label,
                      should_create_partitions, should_align_boot_part=False):
     cylinders = None
     if not media.is_block_device:
-        image_size_in_bytes = convert_size_to_bytes(image_size)
+        image_size_in_bytes = get_partition_size_in_bytes(image_size)
         cylinders = image_size_in_bytes / CYLINDER_SIZE
         proc = cmd_runner.run(
             ['dd', 'of=%s' % media.path,
@@ -131,7 +135,7 @@
     """
     cylinders = None
     if not media.is_block_device:
-        image_size_in_bytes = convert_size_to_bytes(image_size)
+        image_size_in_bytes = get_partition_size_in_bytes(image_size)
         cylinders = image_size_in_bytes / CYLINDER_SIZE
         proc = cmd_runner.run(
             ['dd', 'of=%s' % media.path,
@@ -478,14 +482,20 @@
             path, 'DeviceFile', dbus_interface=DBUS_PROPERTIES))
 
 
-def convert_size_to_bytes(size):
-    """Convert a size string in Kbytes, Mbytes or Gbytes to bytes."""
+def get_partition_size_in_bytes(size):
+    """Convert a size string in Kbytes, Mbytes or Gbytes to bytes.
+
+    The conversion rounds-up the size to the nearest MiB, considering a minimum
+    size of MIN_IMAGE_SIZE bytes. The conversion always assures to have a big
+    enough size for the partition.
+    """
     unit = size[-1].upper()
+    real_size = float(size[:-1])
+
     # no unit? (ends with a digit)
     if unit in '0123456789':
-        return int(round(float(size)))
-    real_size = float(size[:-1])
-    if unit == 'K':
+        real_size = float(size)
+    elif unit == 'K':
         real_size = real_size * 1024
     elif unit == 'M':
         real_size = real_size * 1024 * 1024
@@ -494,8 +504,20 @@
     else:
         raise ValueError("Unknown size format: %s.  Use K[bytes], M[bytes] "
                          "or G[bytes]" % size)
-
-    return int(round(real_size))
+    # Guarantee that is a multiple of ROUND_IMAGE_TO
+    real_size = _check_min_size(int(ceil(real_size / ROUND_IMAGE_TO) *
+                                    ROUND_IMAGE_TO))
+    return real_size
+
+
+def _check_min_size(size):
+    """Check that the image size is at least MIN_IMAGE_SIZE bytes.
+
+    :param size: The size of the image to check, as a number.
+    """
+    if (size < MIN_IMAGE_SIZE):
+        size = MIN_IMAGE_SIZE
+    return size
 
 
 def run_sfdisk_commands(commands, heads, sectors, cylinders, device,

=== modified file 'linaro_image_tools/media_create/tests/test_media_create.py'
--- linaro_image_tools/media_create/tests/test_media_create.py	2012-06-07 13:12:42 +0000
+++ linaro_image_tools/media_create/tests/test_media_create.py	2012-06-13 13:45:20 +0000
@@ -38,11 +38,11 @@
 from linaro_image_tools import cmd_runner
 import linaro_image_tools.media_create
 from linaro_image_tools.media_create import (
+    android_boards,
+    boards,
     check_device,
-    boards,
     partitions,
     rootfs,
-    android_boards,
     )
 from linaro_image_tools.media_create.boards import (
     SECTOR_SIZE,
@@ -64,8 +64,8 @@
     BoardConfig,
     )
 from linaro_image_tools.media_create.android_boards import (
+    AndroidSnowballEmmcConfig,
     android_board_configs,
-    AndroidSnowballEmmcConfig,
     )
 from linaro_image_tools.media_create.chroot_utils import (
     copy_file,
@@ -79,23 +79,25 @@
     )
 from linaro_image_tools.media_create.partitions import (
     HEADS,
+    MIN_IMAGE_SIZE,
+    Media,
     SECTORS,
+    _check_min_size,
+    _get_device_file_for_partition_number,
+    _parse_blkid_output,
+    calculate_android_partition_size_and_offset,
     calculate_partition_size_and_offset,
-    calculate_android_partition_size_and_offset,
-    convert_size_to_bytes,
     create_partitions,
     ensure_partition_is_not_mounted,
+    get_android_loopback_devices,
     get_boot_and_root_loopback_devices,
-    get_android_loopback_devices,
     get_boot_and_root_partitions_for_media,
-    Media,
+    get_partition_size_in_bytes,
+    get_uuid,
     partition_mounted,
     run_sfdisk_commands,
     setup_partitions,
-    get_uuid,
-    _parse_blkid_output,
     wait_partition_to_settle,
-    _get_device_file_for_partition_number,
     )
 from linaro_image_tools.media_create.rootfs import (
     append_to_fstab,
@@ -104,8 +106,8 @@
     move_contents,
     populate_rootfs,
     rootfs_mount_options,
+    update_network_interfaces,
     write_data_to_protected_file,
-    update_network_interfaces,
     )
 from linaro_image_tools.media_create.tests.fixtures import (
     CreateTarballFixture,
@@ -492,7 +494,9 @@
 
         class config(BoardConfig):
             pass
-        self.assertRaises(AssertionError, config.set_metadata, 'ahwpack.tar.gz')
+        self.assertRaises(AssertionError,
+                          config.set_metadata,
+                          'ahwpack.tar.gz')
 
 
 class TestGetMLOFile(TestCaseWithFixtures):
@@ -767,8 +771,11 @@
         uboot_file = os.path.join(uboot_dir, 'u-boot.bin')
         uboot_relative_file = uboot_file.replace(self.tempdir, '')
         with open(cfg_file, 'w') as f:
-                f.write('%s %s %i %#x %s\n' % ('NORMAL', uboot_relative_file, 0,
-                                               0xBA0000, '9'))
+                f.write('%s %s %i %#x %s\n' % ('NORMAL',
+                                               uboot_relative_file,
+                                               0,
+                                               0xBA0000,
+                                               '9'))
         with open(uboot_file, 'w') as f:
             file_info = boards.SnowballEmmcConfig.get_file_info(
                 self.tempdir, self.temp_bootdir_path)
@@ -781,8 +788,10 @@
         with open(cfg_file, 'w') as f:
                 f.write('%s %s %i %#x %s\n' % ('NORMAL', 'u-boot.bin', 0,
                                                0xBA0000, '9'))
-        self.assertRaises(AssertionError, boards.SnowballEmmcConfig.get_file_info,
-                          self.tempdir, self.temp_bootdir_path)
+        self.assertRaises(AssertionError,
+                          boards.SnowballEmmcConfig.get_file_info,
+                          self.tempdir,
+                          self.temp_bootdir_path)
 
     def test_file_name_size(self):
         ''' Test using a to large toc file '''
@@ -961,7 +970,8 @@
         super(TestBootSteps, self).setUp()
         self.funcs_calls = []
         self.mock_all_boards_funcs()
-        linaro_image_tools.media_create.boards.BoardConfig.hwpack_format = '1.0'
+        linaro_image_tools.media_create.boards.BoardConfig.hwpack_format = \
+        '1.0'
 
     def mock_all_boards_funcs(self):
         """Mock functions of boards module with a call tracer."""
@@ -1037,8 +1047,8 @@
             lambda: '1.0')
         self.make_boot_files(boards.SMDKV310Config)
         expected = [
-            'install_samsung_boot_loader', 'make_flashable_env', '_dd', 'make_uImage',
-            'make_uInitrd', 'make_boot_script']
+            'install_samsung_boot_loader', 'make_flashable_env', '_dd',
+            'make_uImage', 'make_uInitrd', 'make_boot_script']
         self.assertEqual(expected, self.funcs_calls)
 
     def test_origen_steps(self):
@@ -1058,8 +1068,8 @@
             lambda: '1.0')
         self.make_boot_files(boards.OrigenConfig)
         expected = [
-            'install_samsung_boot_loader', 'make_flashable_env', '_dd', 'make_uImage',
-            'make_uInitrd', 'make_boot_script']
+            'install_samsung_boot_loader', 'make_flashable_env', '_dd',
+            'make_uImage', 'make_uInitrd', 'make_boot_script']
         self.assertEqual(expected, self.funcs_calls)
 
     def test_ux500_steps(self):
@@ -2424,32 +2434,46 @@
             '98367,-,E\n98367,65536,L\n294975,131072,L\n' \
             '426047,,,-', '%s' % self.android_image_size)
 
+    def test_check_min_size_small(self):
+        """Check that we get back the minimum size as expeceted."""
+        self.assertEqual(MIN_IMAGE_SIZE, _check_min_size(12345))
+
+    def test_check_min_size_big(self):
+        """Check that we get back the same value we pass."""
+        self.assertEqual(MIN_IMAGE_SIZE * 3, _check_min_size(3145728))
+
+    def test_convert_size_wrong_suffix(self):
+        self.assertRaises(ValueError, get_partition_size_in_bytes, "123456H")
+
     def test_convert_size_no_suffix(self):
-        self.assertEqual(524288, convert_size_to_bytes('524288'))
+        self.assertEqual(2 ** 20, get_partition_size_in_bytes('123456'))
+
+    def test_convert_size_one_mbyte(self):
+        self.assertEqual(2 ** 20, get_partition_size_in_bytes('1M'))
 
     def test_convert_size_in_kbytes_to_bytes(self):
-        self.assertEqual(512 * 1024, convert_size_to_bytes('512K'))
+        self.assertEqual(2 * 2 ** 20, get_partition_size_in_bytes('2048K'))
 
     def test_convert_size_in_mbytes_to_bytes(self):
-        self.assertEqual(100 * 1024 ** 2, convert_size_to_bytes('100M'))
+        self.assertEqual(100 * 2 ** 20, get_partition_size_in_bytes('100M'))
 
     def test_convert_size_in_gbytes_to_bytes(self):
-        self.assertEqual(12 * 1024 ** 3, convert_size_to_bytes('12G'))
+        self.assertEqual(12 * 2 ** 30, get_partition_size_in_bytes('12G'))
 
     def test_convert_size_float_no_suffix(self):
-        self.assertEqual(1539, convert_size_to_bytes('1539.49'))
-
-    def test_convert_size_float_round_up(self):
-        self.assertEqual(1540, convert_size_to_bytes('1539.50'))
+        self.assertEqual(3 * 2 ** 20, get_partition_size_in_bytes('2348576.91'))
 
     def test_convert_size_float_in_kbytes_to_bytes(self):
-        self.assertEqual(int(round(234.8 * 1024)), convert_size_to_bytes('234.8K'))
+        self.assertEqual(3 * 2 ** 20, get_partition_size_in_bytes('2345.8K'))
+
+    def test_convert_size_float_in_mbytes_to_bytes_double(self):
+        self.assertEqual(2 * 2 ** 20, get_partition_size_in_bytes('1.0000001M'))
 
     def test_convert_size_float_in_mbytes_to_bytes(self):
-        self.assertEqual(int(round(876.123 * 1024 ** 2)), convert_size_to_bytes('876.123M'))
+        self.assertEqual(877 * 2 ** 20, get_partition_size_in_bytes('876.123M'))
 
     def test_convert_size_float_in_gbytes_to_bytes(self):
-        self.assertEqual(int(round(1.9 * 1024 ** 3)), convert_size_to_bytes('1.9G'))
+        self.assertEqual(1946 * 2 ** 20, get_partition_size_in_bytes('1.9G'))
 
     def test_calculate_partition_size_and_offset(self):
         tmpfile = self._create_tmpfile()
@@ -3400,6 +3424,7 @@
 
         # Ensure the list of cleanup functions gets cleared to make sure tests
         # don't interfere with one another.
+
         def clear_atexits():
             linaro_image_tools.media_create.chroot_utils.local_atexit = []
         self.addCleanup(clear_atexits)