[U-Boot] [PATCH v3 7/8] x86: efi: Add a hello world test program

[Simon Glass]

Hi Alex,

On 14 November 2016 at 13:46, Alexander Graf <agraf at suse.de> wrote:
>
>
> On 14/11/2016 21:44, Simon Glass wrote:
>>
>> Hi Alex,
>>
>> On 11 November 2016 at 23:23, Alexander Graf <agraf at suse.de> wrote:
>>>
>>>
>>>
>>>> Am 11.11.2016 um 17:17 schrieb Simon Glass <sjg at chromium.org>:
>>>>
>>>> Hi Alex,
>>>>
>>>>> On 7 November 2016 at 09:32, Alexander Graf <agraf at suse.de> wrote:
>>>>>
>>>>>
>>>>>> On 07/11/2016 10:46, Simon Glass wrote:
>>>>>>
>>>>>> Hi Alex,
>>>>>>
>>>>>>> On 19 October 2016 at 01:09, Alexander Graf <agraf at suse.de> wrote:
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>> On 18/10/2016 22:37, Simon Glass wrote:
>>>>>>>>
>>>>>>>> Hi Alex,
>>>>>>>>
>>>>>>>>> On 18 October 2016 at 01:14, Alexander Graf <agraf at suse.de> wrote:
>>>>>>>>>
>>>>>>>>>> On 10/18/2016 04:29 AM, Simon Glass wrote:
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> It is useful to have a basic sanity check for EFI loader support.
>>>>>>>>>> Add
>>>>>>>>>> a
>>>>>>>>>> 'bootefi hello' command which loads HelloWord.efi and runs it
>>>>>>>>>> under
>>>>>>>>>> U-Boot.
>>>>>>>>>>
>>>>>>>>>> Signed-off-by: Simon Glass <sjg at chromium.org>
>>>>>>>>>> ---
>>>>>>>>>>
>>>>>>>>>> Changes in v3:
>>>>>>>>>> - Include a link to the program instead of adding it to the tree
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> So, uh, where is the link?
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>> I put it in the README (see the arm patch).
>>>>>>>>
>>>>>>>>>
>>>>>>>>> I'm really not convinced this command buys us anything yet. I do
>>>>>>>>> agree
>>>>>>>>> that
>>>>>>>>> we want automated testing - but can't we get that using QEMU and a
>>>>>>>>> downloadable image file that we pass in as disk and have the distro
>>>>>>>>> boot do
>>>>>>>>> its magic?
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>> That seems very heavyweight as a sanity check, although I agree it
>>>>>>>> is
>>>>>>>> useful.
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> It's not really much more heavy weight. The "image file" could simply
>>>>>>> contain your hello world binary. But with this we don't just verify
>>>>>>> whether "bootefi" works, but also whether the default boot path works
>>>>>>> ok.
>>>>>>
>>>>>>
>>>>>>
>>>>>> I don't think I understand what you mean by 'image file'. Is it
>>>>>> something other than the .efi file? Do you mean a disk image?
>>>>>
>>>>>
>>>>>
>>>>> Yes. For reasonable test coverage, we should also verify that the
>>>>> distro
>>>>> defaults wrote a sane boot script that automatically searches for a
>>>>> default
>>>>> EFI binary in /efi/boot/bootx86.efi on the first partition of all
>>>>> devices
>>>>> and runs it.
>>>>>
>>>>> So if we just provide an SD card image or hard disk image to QEMU which
>>>>> contains a hello world .efi binary as that default boot file, we don't
>>>>> only
>>>>> test whether the "bootefi" command works, but also whether the distro
>>>>> boot
>>>>> script works.
>>>>
>>>>
>>>> That's right.
>>>>
>>>>>
>>>>>>
>>>>>>>
>>>>>>>> Here I am just making sure that EFI programs can start, print output
>>>>>>>> and exit. It is a test that we can easily run without a lot of
>>>>>>>> overhead, much less than a full distro boot.
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> Again, I don't think it's much more overhead and I do believe it
>>>>>>> gives
>>>>>>> us much cleaner separation between responsibilities of code (tests go
>>>>>>> where tests are).
>>>>>>
>>>>>>
>>>>>>
>>>>>> You are talking about a functional test, something that tests things
>>>>>> end to end. I prefer to at least start with a smaller test. Granted it
>>>>>> takes a little more work but it means there are fewer things to hunt
>>>>>> through when something goes wrong.
>>>>>
>>>>>
>>>>>
>>>>> Yes, I personally find unit tests terribly annoying and unproductive
>>>>> and
>>>>> functional tests very helpful :). And in this case, the effort to write
>>>>> it
>>>>> is about the same for both, just that the functional test actually
>>>>> tells you
>>>>> that things work or don't work at the end of the day.
>>>>>
>>>>> With a code base like U-Boot, a simple functional test like the above
>>>>> plus
>>>>> git bisect should get you to an offending patch very quickly.
>>>>
>>>>
>>>> This is not a unit test - in fact the EFI stuff has no unit tests. I
>>>> suppose if we are trying to find a name this is a small functional
>>>> test since it exercises the general functionality.
>>>>
>>>> I am much keener on small tests than large ones for finding simple
>>>> bugs. Of course you can generally bisect to find a bug, but the more
>>>> layers of software you need to look for the harder this is.
>>>>
>>>> We could definitely use a pytest which checks an EFI boot into an
>>>> image, but I don't think this obviates the need for a smaller targeted
>>>> test like this one.
>>>
>>>
>>> I think arguing over this is moot :). More tests is usually a good thing,
>>> so whoever gets to write them gets to push them ;). As long as the licenses
>>> are sound at least.
>>
>>
>> OK good, well please can you review this at some point?
>
>
> Review what exactly?

I mean the patches. There should be ~14 in your queue.

>
>> Also, are you
>> planning to write the 'larger' test? How do you test this all in suse?
>
>
> Planning yes, but I'm very good at not writing tests :).
>
> Currently I'm testing this all in suse by running systems which rely on the
> code to work.

OK I see.

Regards,
Simon