On 03/25/2015 08:55 PM, Paolo Bonzini wrote: > > > On 25/03/2015 10:36, Wen Congyang wrote: >> >> +void backup_do_checkpoint(BlockJob *job, Error **errp) >> +{ >> + BackupBlockJob *backup_job = container_of(job, BackupBlockJob, common); >> + >> + if (job->driver != &backup_job_driver) { >> + error_setg(errp, "It is not backup job"); >> + return; >> + } >> + >> + hbitmap_reset_all(backup_job->bitmap); >> +} > > Please add instead a block_job_do_checkpoint API, and a do_checkpoint > function pointer to BlockJobDriver. > >> +{ >> +#if 0 >> + hbitmap_reset(hb, 0, hb->size << hb->granularity); >> +#else >> + uint64_t size = hb->size; >> + unsigned int i; >> + >> + /* Same as hbitmap_alloc() except memset() */ >> + for (i = HBITMAP_LEVELS; i-- > 0; ) { > > This can be "--i >= 1"... > >> + size = MAX((size + BITS_PER_LONG - 1) >> BITS_PER_LEVEL, 1); >> + memset(hb->levels[i], 0, size * sizeof(unsigned long)); >> + } >> + >> + assert(size == 1); >> + hb->levels[0][0] |= 1UL << (BITS_PER_LONG - 1); > > ... if you use "=" instead of "|=" here. > >> +#endif >> +} > > Please pick one implementation (no #if), and also add a testcase to > tests/test-hbitmap.c.
OK Thanks Wen Congyang > > Paolo > . >