Code review comment for lp:~milo/linaro-image-tools/multiple-boards-support

Hello Danilo,

thanks for the review!

On Sat, Jul 21, 2012 at 8:55 AM, Данило Шеган <email address hidden> wrote:
>
> My main gripe is that I am seeing a huge branch like this. I still
> can't see why this was not done in several branches: one to parse the
> new hwpack config, another to produce the new hwpacks and the third one
> to port l-m-c to use them.
>
> Also, a BP workitems are still out of date I believe.
>
>> === modified file 'linaro-hwpack-convert'
>> --- linaro-hwpack-convert 2012-07-19 15:21:06 +0000
>> +++ linaro-hwpack-convert 2012-07-20 14:12:31 +0000
>> @@ -31,6 +31,7 @@
>> HwpackConverterException,
>> check_and_validate_args,
>> )
>> +from linaro_image_tools.hwpack.builder import ConfigFileMissing
>> from linaro_image_tools.__version__ import __version__
>
> This change looks unnecessary (judging by the diff itself). If it's
> not, something was badly broken before :)

It is unnecessary, indeed.
Fixed and pushed.

>> logger = logging.getLogger(__name__)
>>
>> @@ -110,6 +117,22 @@
>> package.filepath, wanted_file)
>> return hwpack.add_file(target_path, tempfile_name)
>>
>> + def loop_bootloaders(self, dictionary):
>> + """Loop through the bootloaders dictionary searching for packages
>> + that should be installed, based on known keywords.
>> +
>> + :param dictionary: The bootloaders dictionary to loop through.
>
> Is the "dictionary" actually a bootloaders config section? If it is,
> what do you think of naming the method argument "bootloaders_config"?

Yes, it is the bootloaders section. Renamed to clearly state that.

>> + :return A list of packages, without duplicates."""
>> + boot_packages = []
>> + for key, value in dictionary.iteritems():
>> + if isinstance(value, dict):
>> + boot_packages.extend(self.loop_bootloaders(value))
>
> Wow, do we actually support recursive nesting of bootloaders?

Hmmm... kind of. The YAML structure could be something like this too:

boards:
 a_board:
  bootloaders: # this is specific for the board
   u_boot:
    file: a_file
bootloaders: # this is external, and is general for the whole hwpack config
 uefi:
  pacakge: a
 uboot:
  package: b

so we have multiple bootloaders section, each more specific than the others.

> Also,
> this method would probably be better named "find_bootloader_packages".

Done, and pushed.

>> === modified file 'linaro_image_tools/hwpack/hardwarepack.py'
>> @@ -127,6 +166,10 @@
>> self.samsung_bl2_len = samsung_bl2_len
>>
>> @classmethod
>> + def add_v3_config(self, bootloaders):
>> + self.bootloaders = bootloaders
>
> This is entirely unclear. What's going on here?

Basically, that method is called only during the construction of the
metadata file, starting from a Config object, during the build of the
hwpack archive. The args of that method should be specific to the v3
of the hwpack config.
If we are using a v3 hwpack config converted from a v2 config, we
might have a bootloaders section in the file (since it will not have a
boards section).

> Please add a docstring. Btw, are not the number of boards another new
> thing here?

Docstring added, for the boards part that it was missing, I created
another branch. Will merge it later after this one has been reviewed.

>> + def loop_through_dictionary(self, dictionary, search_key, new_value):
>> + """Loop recursively thorugh a dictionary looking for the specified key
>> + substituting its value.
>
> A typo: "thorugh".
>
> I dislike the name here as well. What this does is set a config value
> in a recursive dict structure. How about we name it along those lines?
>
> Also, I wonder how this works when we do have multiple bootloaders
> defined, eg.
>
> bootloaders:
> uefi1:
> boot_package: blah
> uefi2:
> boot_package: foo
>
> and we try to set the value of "boot_package"?
>
> (similar probably holds for the loop_bootloader method all the way at
> the top of the diff)

The previous loop should not be affected by this problem, since what
we need from that, is just a list of the packages that need to be
downloaded. In this one we need to change values.

>> +
>> + :param dictionary: The dictionary to loop through.
>> + :param serach_key: The key to search.
>
> "search_key".
>
>> + :param new_value: The new value for the key.
>> + """
>> + # XXX Probably better check what should happen if the key we are
>> + # looking for is not there, but the value is set. Should we set it
>> + # anyway? If yes, where?
>
> I am not sure how can a value be set if the key is not there.

Since we are changing basically two predefined fields at the moment
(spl and u_boot), my fear is that someone places the fields outside of
the bootloaders section (mostly the spl one), and when we check and
loop:
- the value is set,
- but the key is not in our dictionary of the bootloaders, where it should be

We do not really have a "checker" or validator for a hwpackv3 YAML based file.

Another problem I see with this one, is that the Config class does not
really have a clear separation between v2 and v3 style (and the same
happens with metadata; metadata that could be easily included in the
Config, since it is a representation of that), and we are carrying
information of both version in just one class. In v2, those values are
just normal values in a "plain" file, but in v3 they are part of the
bootloaders section (nested).

What happens here is that when creating the hwpack archive, and
writing the metadata file, those values are re-set, directly inside
the metadata, with a new "translated" value.

So, we start from a hwpack config file with something like this:
spl_file = usr/lib/u-boot/omap4_panda/MLO

And we end up in the metadata file with this:
spl_file = spl/MLO
(and in the old metadata this was converted into 'spl=spl/MLO',
translating also the key name, not just the value)

>
>> + for key, value in dictionary.iteritems():
>> + if key == search_key:
>> + dictionary[key] = new_value
>> + break
>> + elif isinstance(value, dict):
>> + self.loop_through_dictionary(value, search_key, new_value)
>
> Judging by the use of it, perhaps we don't have to guard against the
> problems I mention above, but then it is most important to get at least
> some documentation about this in the docstring.

Actually, we should guard.
I reworked the section in a separate branch, and instead of looping at
that point, we loop directly in the builder, convert the values there,
and re-set the metadata variable.

Will address the other points in a separate mail.

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