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

[Stefan Hajnoczi]

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.

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

> +        }
> +    }
> +    g_free(buf);

qemu_blockalign() memory is freed with qemu_vfree(), not g_free().

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

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

signature.asc
Description: PGP signature