[RFC,v2,4/4] tests: add some sdcard qtest

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 tests/sdbus-test.c     | 151 +++++++++++++++++++++++++++++++++++++++++++++++++
 tests/Makefile.include |   2 +
 2 files changed, 153 insertions(+)
 create mode 100644 tests/sdbus-test.c

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?

> +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?

> +    // 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.

> +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.

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.

On Fri, Jan 05, 2018 at 12:55:41PM -0300, Philippe Mathieu-Daudé wrote:
> 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 :)

If you want to commit commented out code then please include a comment
that explains why it's commented out.  That way readers understand why
it's there.

Also please run checkpatch.pl against all patches.  It can be run as a
commit hook so all your code is checked at commit time:
http://blog.vmsplice.net/2011/03/how-to-automatically-run-checkpatchpl.html