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

Thu Jun 28 13:42:00 UTC 2018

Hi Stephen,

On Wed, Jun 27, 2018 at 10:07:04AM -0600, Stephen Warren wrote:
> 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.
> 

Thanks for the explanation, hopefully that's the only "bug".

> 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).
> 

I took 2MiB as told.

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

Done.

> 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:
> 

Done.

> 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.

I'm running all tests with and without my patch series for environment
variable whitelisting, let's see the outcome tomorrow.

Thanks,
Quentin
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 801 bytes
Desc: not available
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20180628/7dad134d/attachment.sig>