[v2,8/8] qtest: Add boot order test

Covers only PC so far.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 tests/Makefile          |  2 ++
 tests/boot-order-test.c | 71 +++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 73 insertions(+)
 create mode 100644 tests/boot-order-test.c

Am 22.02.2013 18:20, schrieb Markus Armbruster:
> Covers only PC so far.
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>

Reviewed-by: Andreas Färber <afaerber@suse.de>

> +static void test_cmos_byte(int reg, int expected)
> +{
> +    int actual;
> +
> +    outb(0x70 + 0, reg);
> +    actual = inb(0x71);

Did you intend to use 0x70 + 1 here or why the 0x70 + 0 above?

> +    g_assert_cmphex(actual, ==, expected);
> +}

> +int main(int argc, char *argv[])
> +{
> +    g_test_init(&argc, &argv, NULL);
> +
> +    qtest_add_func("boot-order/pc", test_pc_boot_order);
> +
> +    return g_test_run();
> +}

So this is suggesting to me to add separate test functions per arch.
Fine with me, we'll only have to reindent one line then.

Thanks for the changes,

Andreas

Andreas Färber <afaerber@suse.de> writes:

> Am 22.02.2013 18:20, schrieb Markus Armbruster:
>> Covers only PC so far.
>> 
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>
> Reviewed-by: Andreas Färber <afaerber@suse.de>
>
>> +static void test_cmos_byte(int reg, int expected)
>> +{
>> +    int actual;
>> +
>> +    outb(0x70 + 0, reg);
>> +    actual = inb(0x71);
>
> Did you intend to use 0x70 + 1 here or why the 0x70 + 0 above?

Looks like a pasto to me.  Let's drop the stupid '+ 0'.

>> +    g_assert_cmphex(actual, ==, expected);
>> +}
>
>> +int main(int argc, char *argv[])
>> +{
>> +    g_test_init(&argc, &argv, NULL);
>> +
>> +    qtest_add_func("boot-order/pc", test_pc_boot_order);
>> +
>> +    return g_test_run();
>> +}
>
> So this is suggesting to me to add separate test functions per arch.
> Fine with me, we'll only have to reindent one line then.
>
> Thanks for the changes,

Hard to predict for me how the tests for other archs will look, and what
could be shared.  Add them and see :)

Am 22.02.2013 18:20, schrieb Markus Armbruster:
> diff --git a/tests/boot-order-test.c b/tests/boot-order-test.c
> new file mode 100644
> index 0000000..60412ad
> --- /dev/null
> +++ b/tests/boot-order-test.c
[...]
> +static void test_pc_with_args(const char *args, uint8_t boot1, uint8_t boot2,
> +                              uint8_t reboot1, uint8_t reboot2)
> +{
> +    char *extra_args = g_strdup_printf("-nodefaults -display none -S %s",
> +                                       args);

Why do you add -S here? qtest is an accel mode of its own and should
never execute anything.

Also in my code I found it more logical to add extra args to args rather
than the reverse, not just because of 80 chars limit when trying to add
-machine argument. ;)

I assume q35 is reusing pc code? Or should it be tested as well?
(Having any of our qtests cover it would be nice.)

> +    qtest_start(extra_args);
> +    test_cmos(boot1, boot2);

> +    qtest_qmp(global_qtest, "{ 'execute': 'system_reset' }");

qmp()?

> +    test_cmos(reboot1, reboot2);
> +    qtest_quit(global_qtest);
> +    g_free(extra_args);
> +}
[snip]

Andreas

Andreas Färber <afaerber@suse.de> writes:

> Am 22.02.2013 18:20, schrieb Markus Armbruster:
>> diff --git a/tests/boot-order-test.c b/tests/boot-order-test.c
>> new file mode 100644
>> index 0000000..60412ad
>> --- /dev/null
>> +++ b/tests/boot-order-test.c
> [...]
>> +static void test_pc_with_args(const char *args, uint8_t boot1, uint8_t boot2,
>> +                              uint8_t reboot1, uint8_t reboot2)
>> +{
>> +    char *extra_args = g_strdup_printf("-nodefaults -display none -S %s",
>> +                                       args);
>
> Why do you add -S here? qtest is an accel mode of its own and should
> never execute anything.

Brain fart, dropping.

> Also in my code I found it more logical to add extra args to args rather
> than the reverse, not just because of 80 chars limit when trying to add
> -machine argument. ;)

Point taken :)

> I assume q35 is reusing pc code? Or should it be tested as well?
> (Having any of our qtests cover it would be nice.)

They share the RTC CMOS setup.

>> +    qtest_start(extra_args);
>> +    test_cmos(boot1, boot2);
>
>> +    qtest_qmp(global_qtest, "{ 'execute': 'system_reset' }");
>
> qmp()?

Yes.

>> +    test_cmos(reboot1, reboot2);
>> +    qtest_quit(global_qtest);
>> +    g_free(extra_args);
>> +}
> [snip]

Thanks!