On Tue, Oct 24, 2017 at 11:23:06PM +0200, Claudius Heine wrote: > Hi Andreas, > > On Tue, 2017-10-24 at 17:47 +0200, Andreas Reichel wrote: > > On Tue, Oct 24, 2017 at 05:23:07PM +0200, Claudius Heine wrote: > > > On Tue, 2017-10-24 at 14:26 +0200, Andreas J. Reichel wrote: > > > > From: Andreas Reichel <[email protected]> > > > > > > > > The new testcode initially provides tests for > > > > * internal userspace API core functions > > > > * external userspace API functions (libebgenv.a) > > > > > > > > They are devided into the following test-suits: > > > > > > > > * test_bgenv_init_retval.c, where partition probing must be > > > > faked and disks must be simualted. > > > > * test_probe_config_file.c, where the existence of environment > > > > data is faked for simulated config partitions > > > > * test_ebgenv_api_internal.c, core functions for userspace API > > > > * test_ebgenv_api.c, library API functions for userspace > > > > > > This is a pretty large patch. Maybe split this patch up a bit? One > > > patch for the empty test setup (test_main and Makefile.am), one for > > > utility functions (fake_devices), and then one patch for each set > > > of > > > tests. > > > > Splitting the test code does not make it shorter. > > Agree. > > > If a reviewer reads > > 2000 lines of test-code in one file or 2000 lines of test-code in > > several files... > > Yes? What then? > > Reading 2000 lines in several files is easier, because they are sorted > together, hopefully logical/structural. Also multiple patches that each > contain 1 header file and 1 implementation file is easier to read than > 1 patch that contains multiple files because you don't have to jump > around much to see how they fit together. > I personally do not get this. For me it is always easier to see everything in place instead of split up. But if you like it split up I can split it up...
> > I don't like to have 5 patch files because the test > > code has to be applied finally. If it is not okay, I will fix it. > > I don't understand you here. Could you elaborate? > It happens that patch series are applied partially. That was another thought. But I'm okay with redoing this. > > This patch does nothing else but add the tests that are based on the > > patches before. > > They were rewritten to fit a new test infrastructure, were they not? So > its not just moving code around unchanged? If so they should be > reviewed. > Add code means "add new code". Of course, please. > You do know that I do this review not to anger you, but to help > accelerate this process. Jan does not always has time to fully review I am neither upset nor angry nor anything else... I just do not agree with some opinions :) If I would, there were no discussions and solutions are never optimal without discussions. I am always thankful for your reviews. But that does not change my technical thoughts on certain things. > everything, so I help you and him by doing some of it. This should > normally lead to a faster patch development cycle. > > > > > > > Not sure about the removal of the static. I see two options, one > > > patch > > > that removes all the 'static' or remove the 'static' within the > > > patches > > > that use those functions. > > > > > > > I am quite sure. I remove the static keywoards in this patch, because > > the tests need this. And the tests are introduced by this patch. > > Makes > > it easier to review. > > So you go with option 2 then. But if you split this patch up, then it > might be more work for you. Just saying... > > Also I have some more review comments in the rest of the last mail that > might have gone unnoticed. > Just a note: If I do not answer something, I usually agree or have not yet come to an opinion. > I will try to get through the rest of it at a later date. > Thank you! Andreas > Cheers, > Claudius > > -- > DENX Software Engineering GmbH, Managing Director: Wolfgang Denk > HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany > Phone: (+49)-8142-66989-54 Fax: (+49)-8142-66989-80 Email: [email protected] > > PGP key: 6FF2 E59F 00C6 BC28 31D8 64C1 1173 CB19 9808 B153 > Keyserver: hkp://pool.sks-keyservers.net -- Andreas Reichel Dipl.-Phys. (Univ.) Software Consultant [email protected], +49-174-3180074 TNG Technology Consulting GmbH, Betastr. 13a, 85774 Unterfoehring Geschaeftsfuehrer: Henrik Klagges, Dr. Robert Dahlke, Gerhard Mueller Sitz: Unterfoehring * Amtsgericht Muenchen * HRB 135082 -- You received this message because you are subscribed to the Google Groups "EFI Boot Guard" group. To unsubscribe from this group and stop receiving emails from it, send an email to [email protected]. To post to this group, send email to [email protected]. To view this discussion on the web visit https://groups.google.com/d/msgid/efibootguard-dev/20171025084939.GA19902%40iiotirae. For more options, visit https://groups.google.com/d/optout.
