From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:51899) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1b7gkX-000698-6M for qemu-devel@nongnu.org; Tue, 31 May 2016 06:18:16 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1b7gkV-000199-2e for qemu-devel@nongnu.org; Tue, 31 May 2016 06:18:12 -0400 Message-ID: <574D65C0.2080109@cn.fujitsu.com> Date: Tue, 31 May 2016 18:21:52 +0800 From: Changlong Xie MIME-Version: 1.0 References: <1463729780-31982-1-git-send-email-xiecl.fnst@cn.fujitsu.com> <1463729780-31982-10-git-send-email-xiecl.fnst@cn.fujitsu.com> <20160530173429.GA1366@stefanha-x1.localdomain> In-Reply-To: <20160530173429.GA1366@stefanha-x1.localdomain> Content-Type: text/plain; charset="utf-8"; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [Qemu-block] [PATCH v19 09/10] tests: add unit test case for replication List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Stefan Hajnoczi Cc: qemu devel , Stefan Hajnoczi , Fam Zheng , Max Reitz , Kevin Wolf , Jeff Cody , Wen Congyang , zhanghailiang , qemu block , Jiang Yunhong , Dong Eddie , "Dr. David Alan Gilbert" , Markus Armbruster , Gonglei , Paolo Bonzini 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. >