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.  Why not make it 'is at least MIN_IMAGE_SIZE bytes.'
instead?

> === modified file 'linaro_image_tools/media_create/tests/test_media_create.py'

These are not of your doing, but id' be worth fixing them (and I did
notice other fixes you did — thanks! :)

$ pep8 linaro_image_tools/media_create/tests/test_media_create.py
linaro_image_tools/media_create/tests/test_media_create.py:306:9: E301
expected 1 blank line, found 0
linaro_image_tools/media_create/tests/test_media_create.py:484:80: E501
line too long (80 characters)
linaro_image_tools/media_create/tests/test_media_create.py:1096:1: E302
expected 2 blank lines, found 1
linaro_image_tools/media_create/tests/test_media_create.py:2350:42: E225
missing whitespace around operator

Otherwise, I am generally very happy with all the test additions, thanks
again.

> @@ -2409,19 +2417,20 @@
>          self.assertEqual(12 * 1024**3, convert_size_to_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(int(round(floor(2348576.91 / 2**20) * 2**20)),
> +                         convert_size_to_bytes('2348576.91'))

This test would read much better if it simply said 2 * 2**20 in expected
value (though, with my suggestions, it should end up being 3 * 2**20).

Similarly in the follow-up tests: let's make the expected value much
clearer now that rounding is happening.

>      def test_convert_size_float_in_kbytes_to_bytes(self):
>...
>      def test_convert_size_float_in_mbytes_to_bytes(self):
> ... 
>      def test_convert_size_float_in_gbytes_to_bytes(self):
> ... 

 review needs-fixing