Re: [Qemu-devel] [RFC PATCH v2 4/4] tests: add some sdcard qtest

[Philippe Mathieu-Daudé]

Hi Stefan,

On 01/05/2018 12:31 PM, Stefan Hajnoczi wrote:
> On Wed, Jan 03, 2018 at 06:49:25PM -0300, Philippe Mathieu-Daudé wrote:
>> +static const char *machines[PROTO_COUNT] = {
>> +    [PROTO_SD] = "nuri",
>> +    //[PROTO_MMC] = "vexpress-a9",
>> +    //[PROTO_SPI] = "lm3s6965evb"
>> +};
>> +
>> +static const uint64_t sizes[] = {
>> +    //512 * M_BYTE,
>> +    //1 * G_BYTE,
>> +    4 * G_BYTE,
>> +    //64 * G_BYTE,
>> +};
> 
> Why are these commented out?

As I didn't feel good feedback for the previous Python qtests, I
prefered to send this as RFC and to see if this was the good way to go
before spending more time learning QDECREF() and friends :)

However I again forgot to replace RFC -> NOTFORMERGE in the subject :(

These parameters are commented out because the current sd.c code doesn't
work with those cases :(
So to show the qtests is useful and pass Travis/Patchew build, they are
commented, and to show the current model is broken (until I succeed to
fix it), one can uncomment these and see failing tests :)

> 
>> +static void test1(SDBusAdapter *mmc, uint64_t size)
>> +{
>> +    uint8_t *response;
>> +    uint16_t rca;
>> +    ssize_t sz;
>> +
>> +    sz = sdbus_do_cmd(mmc, GO_IDLE_STATE, 0, NULL);
>> +    g_assert_cmpuint(sz, ==, 0);
>> +
>> +    sz = sdbus_do_cmd(mmc, SEND_IF_COND, 0x1aa, NULL);
>> +    //g_assert_cmpuint(sz, ==, 0);
> 
> Why is this commented out?

Actually this test is SD oriented, in MMC mode this command is incorrect.
I'll split this test in 3 (the SD/MMC/SPI protocols).

> 
>> +    // TODO 8x: sdcard_read_data len 512
>> +
>> +    //sz = sdbus_do_acmd(mmc, SEND_STATUS, 0, rca, &response);
>> +    //g_free(response);
> 
> Why is there commenteded out code and TODO here?  Please either
> implement this stuff or remove it from the patch.

Yes, I have to handle this differently for MMC vs SD/SPI.

> 
>> +int main(int argc, char **argv)
>> +{
>> +    const char *arch = qtest_get_arch();
>> +    int iproto, isize;
>> +    gchar *path;
>> +    TestCase *test;
>> +
>> +    g_test_init(&argc, &argv, NULL);
>> +
>> +    if (strcmp(arch, "arm") == 0 || strcmp(arch, "aarch64") == 0) {
>> +        for (iproto = 0; iproto < PROTO_COUNT; iproto++) {
>> +            if (!machines[iproto]) {
>> +                continue;
>> +            }
>> +            for (isize = 0; isize < ARRAY_SIZE(sizes); isize++) {
>> +                test = g_new(TestCase, 1);
>> +
>> +                test->protocol = iproto;
>> +                test->size = sizes[isize];
>> +
>> +                path = g_strdup_printf("sdcard/%s/%lu", proto_name[iproto], 
>> sizes[isize]);
>> +                qtest_add_data_func(path, test, sdcard_tests);
>> +                g_free(path);
>> +                // g_free(test)?
> 
> Please remove this.  qtest_add_data_func() keeps the pointer to test so
> it must not be freed.

Ok!

Thanks for your review :)

Phil.

signature.asc
Description: OpenPGP digital signature