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

[Changlong Xie]

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.

>