[U-Boot] [U-Boot, v4, 2/2] test/py: add test for whitelist of variables while importing environment

[Stephen Warren]

On 06/27/2018 09:23 AM, Tom Rini wrote:
> On Wed, Jun 27, 2018 at 09:52:50AM +0200, Quentin Schulz wrote:
>> Hi Tom,
>>
>> On Tue, Jun 26, 2018 at 10:44:59AM -0400, Tom Rini wrote:
>>> On Tue, Jun 26, 2018 at 02:41:40PM +0200, Quentin Schulz wrote:
>>>> Hi all,
>>>>
>>>> On Wed, Jun 13, 2018 at 01:02:06PM -0600, Stephen Warren wrote:
>>>>> On 06/13/2018 12:53 PM, Quentin Schulz wrote:
>>>>>> Hi Tom,
>>>>>>
>>>>>> On Wed, Jun 13, 2018 at 11:43:32AM -0400, Tom Rini wrote:
>>>>>>> On Mon, Jun 04, 2018 at 11:47:30AM +0200, Quentin Schulz wrote:
>>>>>>>
>>>>>>>> This tests that the importing of an environment with a specified
>>>>>>>> whitelist works as intended.
>>>>>>>>
>>>>>>>> If there are variables passed as parameter to the env import command,
>>>>>>>>      those only should be imported in the current environment.
>>>>>>>>
>>>>>>>> For each variable passed as parameter, if
>>>>>>>>    - foo is bar in current env and bar2 in exported env, after importing
>>>>>>>>    exported env, foo shall be bar2,
>>>>>>>>    - foo does not exist in current env and foo is bar2 in exported env,
>>>>>>>>    after importing exported env, foo shall be bar2,
>>>>>>>>    - foo is bar in current env and does not exist in exported env (but is
>>>>>>>>    passed as parameter), after importing exported env, foo shall be empty,
>>>>>>>>
>>>>>>>> Any variable not passed as parameter should be left untouched.
>>>>>>>>
>>>>>>>> Two other tests are made to test that size cannot be '-' if the checksum
>>>>>>>> protection is enabled.
>>>>>>>>
>>>>>>>> Signed-off-by: Quentin Schulz <quentin.schulz at bootlin.com>
>>>>>>>> Reviewed-by: Simon Glass <sjg at chromium.org>
>>>>>>>> Reviewed-by: Stephen Warren <swarren at nvidia.com>
>>>>>>>
>>>>>>> This seems to not be working?
>>>>>>>
>>>>>>> https://travis-ci.org/trini/u-boot/jobs/391504525
>>>>>>>
>>>>>>
>>>>>> I just rebased on top of v2018.07-rc1, ran
>>>>>> make mrproper
>>>>>> ./test/py/test.py --bd sandbox --build
>>>>>>
>>>>>> and the tests run fine ...
>>>>>
>>>>> Most likely the failure is due to the test relying on some feature that
>>>>> isn't enabled on the board being tested (emulated via qemu); you'll need to
>>>>> add something like the following to indicate which feature the test relies
>>>>> upon:
>>>>>
>>>>> @pytest.mark.buildconfigspec('cmd_echo')
>>>>>
>>>>
>>>> OK, I've added the dependency on cmd_importenv and cmd_exportenv, but
>>>> that does not make it work any better.
>>>>
>>>> I added my U-Boot repo to Travis and ran the tests. Here is the output
>>>> of the job: https://travis-ci.org/QSchulz/u-boot/
>>>>
>>>> Specifically, you have:
>>>> https://travis-ci.org/QSchulz/u-boot/jobs/396742661
>>>> https://travis-ci.org/QSchulz/u-boot/jobs/396742668
>>>> https://travis-ci.org/QSchulz/u-boot/jobs/396742669
>>>> https://travis-ci.org/QSchulz/u-boot/jobs/396742670
>>>> https://travis-ci.org/QSchulz/u-boot/jobs/396742671
>>>>
>>>> I've dumped the RAM after the `env export` and it looks pretty much
>>>> empty compared to what I could see with sandbox tests.
>>>>
>>>> Since all the other tests work, I'm not sure I actually introduced a
>>>> regression or if it just never worked. I'll run tests without my patches
>>>> that do a `env export` followed by a dump of the memory, a reset of the
>>>> environement and a `env import` to see where we stand right now.
>>>
>>> It's possible you've exposed a latent bug here.  What stood out to me at
>>> the time was that it looked like we were using 0x0 for the address and
>>> that's quite possibly an invalid location to be using on these
>>> platforms.
>>>
>>
>> Yes, looked weird to me as well but can't tell if that's a legit RAM
>> address. Stephen says other tests interacting with RAM works on those
>> configs so that should be working.
>>>
>> So, I tested without my patches for whitelisting and it does not work
>> for the same configs:
>>
>> https://travis-ci.org/QSchulz/u-boot/jobs/396898590
>> https://travis-ci.org/QSchulz/u-boot/jobs/396898597
>> https://travis-ci.org/QSchulz/u-boot/jobs/396898598
>> https://travis-ci.org/QSchulz/u-boot/jobs/396898599
>>
>> I'm not introducing a regression with my patch. Do we know if env import
>> and export is actually supported by those configs? How could we fix
>> those? Let me know if I can help.
> 
> Maybe the tests need to depend on something else too then, that's not
> enabled on these platforms?

The bug is in the implementation of hexport_r(); if the provided 
destination pointer is NULL, it allocates a new buffer to write to and 
returns that instead of writing to address 0.

I wonder if some related issue is why test_net adds 4MB to the value 
returned by find_ram_base(); to avoid the NULL pointer.

I'd suggest the following:

1) Enhance find_ram_base() so that if it would return 0, it actually 
returns something else. Adding 2MiB should be safe I think, and return a 
value that's still nicely aligned to any reasonable requirement. (2MiB 
being an ARM LPAE/v8 section size, which makes 2MiB more interesting 
than just 1MiB).

2) Once that's done, perhaps revert the addition of 4MiB in test_net.

3) In the first log you linked above:

https://travis-ci.org/QSchulz/u-boot/jobs/396898590

I see:

Integrator-CP # Integrator-CP # env import 00000000
## Warning: defaulting to text format
## Warning: Input data exceeds 1048576 bytes - truncated
## Info: input data size = 1048578 = 0x100002
Environment import failed: errno = 12

This means "env import" failed, but test/py failed to detect it. Let's 
update the implementation of do_env_export so that its error message is 
formatted like other error messages so that test/py's pattern-based 
error message detection detects it:

u_boot_console_base.py:
pattern_error_notification = re.compile('## Error: ')

Or, update test_env.py to validate that 'Environment import failed' 
isn't in the output of the "env export" command.

After that, hopefully your new test will work on these platforms.