Re: [Qemu-devel] [Qemu-block] [PATCH v19 09/10] tests: add unit test case for replication

[Changlong Xie <xiecl.fnst@cn.fujitsu.com>]

On 05/31/2016 01:34 AM, Stefan Hajnoczi wrote:
> On Fri, May 20, 2016 at 03:36:19PM +0800, Changlong Xie wrote:
>> +/* primary */
>> +#define P_LOCAL_DISK "/tmp/p_local_disk.XXXXXX"
>> +#define P_COMMAND "driver=replication,mode=primary,node-name=xxx,"\
>> +                  "file.driver=qcow2,file.file.filename="P_LOCAL_DISK
>> +
>> +/* secondary */
>> +#define S_LOCAL_DISK "/tmp/s_local_disk.XXXXXX"
>> +#define S_ACTIVE_DISK "/tmp/s_active_disk.XXXXXX"
>> +#define S_HIDDEN_DISK "/tmp/s_hidden_disk.XXXXXX"
>
> Please use unique filenames so that multiple instances of the test can
> run in parallel on a single machine.  mkstemp(3) can be used to do this.
>

will fix in next version.

>> +static void io_read(BlockDriverState *bs, long pattern, int64_t pattern_offset,
>> +                    int64_t pattern_count, int64_t offset, int64_t count,
>> +                    bool expect_failed)
>> +{
>> +    char *buf;
>> +    void *cmp_buf;
>> +    int ret;
>> +
>> +    /* 1. alloc pattern buffer */
>> +    if (pattern) {
>> +        cmp_buf = g_malloc(pattern_count);
>> +        memset(cmp_buf, pattern, pattern_count);
>> +    }
>> +
>> +    /* 2. alloc read buffer */
>> +    buf = qemu_blockalign(bs, count);
>> +    memset(buf, 0xab, count);
>> +
>> +    /* 3. do read */
>> +    ret = bdrv_read(bs, offset >> 9, (uint8_t *)buf, count >> 9);
>> +
>> +    /* 4. assert and compare buf */
>> +    if (expect_failed) {
>> +        g_assert(ret < 0);
>> +    } else {
>> +        g_assert(ret >= 0);
>> +        if (pattern) {
>> +            g_assert(memcmp(buf + pattern_offset, cmp_buf, pattern_count) <= 0);
>> +            g_free(cmp_buf);
>
> if pattern && expect_failed then cmp_buf is leaked.  Probably best to
> initialize cmp_buf = NULL and have an unconditional g_free(cmp_buf) at
> the end of the function to avoid leaks.
>

Yes, you are right.

>> +        }
>> +    }
>> +    g_free(buf);
>
> qemu_blockalign() memory is freed with qemu_vfree(), not g_free().
>

will fix

>> +static void test_primary_do_checkpoint(void)
>> +{
>> +    BlockDriverState *bs;
>> +    Error *local_err = NULL;
>> +
>> +    bs = start_primary();
>> +
>> +    replication_do_checkpoint_all(&local_err);
>> +    g_assert(!local_err);
>> +
>> +    teardown_primary(bs);
>> +}
>
> Shouldn't replication_start_all() be called before
> replication_do_checkpoint_all()?
>

It seems i missed it.

>> +int main(int argc, char **argv)
>> +{
>> +    int ret;
>> +    qemu_init_main_loop(&error_fatal);
>> +    bdrv_init();
>> +
>> +    do {} while (g_main_context_iteration(NULL, false));
>
> Why is this necessary?

Will remove it.

>