Re: [Qemu-devel] [PATCH v2 4/4] i440fx-test: verify firmware under 4G and 1M, both -bios and -pflash

[Laszlo Ersek]

On 11/29/13 15:09, Markus Armbruster wrote:
> Laszlo Ersek <address@hidden> writes:

>> +    /* check below 4G */
>> +    buf = g_malloc0(FW_SIZE);
>> +    memread(0x100000000ULL - FW_SIZE, buf, FW_SIZE);
> 
> Zero-fill immediately followed by read.  Suggest g_malloc().

This was intentional. Please see the preexistent use of memread() in
this file, in verify_area().

> 
>> +    for (i = 0; i < FW_SIZE; ++i) {
>> +        g_assert_cmphex(buf[i], ==, (char unsigned)i);
> 
> (unsigned char), please.
> 
>> +    }
>> +
>> +    /* check in ISA space too */
>> +    memset(buf, 0, FW_SIZE);
>> +    isa_bios_size = ISA_BIOS_MAXSZ < FW_SIZE ? ISA_BIOS_MAXSZ : FW_SIZE;
>> +    memread(0x100000 - isa_bios_size, buf, isa_bios_size);
> 
> Zero-fill immediately followed by read.  Suggest to drop memset().

Same as above. memread() is unable to report errors. Some C library
functions also require you to set errno to zero first, then call the
function, then check errno, because some of the return values are
overlapped by success and failure returns. For memread() there's no
distinction in return value at all.

> 
>> +    for (i = 0; i < isa_bios_size; ++i) {
>> +        g_assert_cmphex(buf[i], ==,
>> +                        (char unsigned)((FW_SIZE - isa_bios_size) + i));
> 
> Again.
> 
>> +    }
>> +
>> +    g_free(buf);
>> +    qtest_end();
>> +}
>> +
>> +static void add_firmware_test(const char *testpath,
>> +                              void (*setup_fixture)(FirmwareTestFixture *f,
>> +                                                    gconstpointer 
>> test_data))
>> +{
>> +    g_test_add(testpath, FirmwareTestFixture, NULL, setup_fixture,
>> +               test_i440fx_firmware, NULL);
>> +}
>> +
>> +static void request_bios(FirmwareTestFixture *fixture,
>> +                         gconstpointer user_data)
>> +{
>> +    fixture->is_bios = true;
>> +}
>> +
>> +static void request_pflash(FirmwareTestFixture *fixture,
>> +                           gconstpointer user_data)
>> +{
>> +    fixture->is_bios = false;
>> +}
>> +
> 
> I'm not sure fixtures are justified here.  Perhaps having the test
> function's test data argument point to the flag would be simpler.

I gave a lot of thought to this. Fixtures are needed. See the
explanation in patch#2.

In more detail here: the gtest framework operates in two distinct
phases. First, you set up all of your test cases in *declarative* style.
That's what all the g_test_add_*() invocations do. You need to describe
everything in advance.

Then, you call g_test_run(), and it runs your declarative script in one
atomic sequence. You cannot change stuff *between* tests. If I want to
run the same test function twice, but with different data, I have the
following choices:

- Allocate and initialize two distinct memory areas. The lifetimes of
both of these will be identical, and they will both live during the
entire test series. I configure the first test case with the address of
the first area, and the 2nd case with the address of the 2nd area.

- Keep one global (shared) area, and let the tests read and write it as
they are executed by the framework. If you reorder the tests in the
outer script, things break.

- Use fixtures. The framework allocates the area for you before each
test, calls your init function on it, runs the test, then tears down the
area. No dependency between tests. Also, if you have N cases in your
test series, you still allocate at most 1 area at any point (as opposed
to at most N, like under option #1).

> 
>> +int main(int argc, char **argv)
>> +{
>> +    TestData data;
>> +    int ret;
>> +
>> +    g_test_init(&argc, &argv, NULL);
>> +
>>      data.num_cpus = 1;
>>  
>>      g_test_add_data_func("/i440fx/defaults", &data, test_i440fx_defaults);
>>      g_test_add_data_func("/i440fx/pam", &data, test_i440fx_pam);
>> +    add_firmware_test("/i440fx/firmware/bios", request_bios);
>> +    add_firmware_test("/i440fx/firmware/pflash", request_pflash);
>>  
>>      ret = g_test_run();
>>      return ret;

Thanks
Laszlo