On Mon, Apr 22, 2013 at 02:59:10PM +0800, Liu Yuan wrote: > +static coroutine_fn int > +sd_co_is_allocated(BlockDriverState *bs, int64_t sector_num, int nb_sectors, > + int *pnum) > +{ > + BDRVSheepdogState *s = bs->opaque; > + SheepdogInode *inode = &s->inode; > + unsigned long start = sector_num * BDRV_SECTOR_SIZE / SD_DATA_OBJ_SIZE, > idx, > + end = DIV_ROUND_UP((sector_num + nb_sectors) * > + BDRV_SECTOR_SIZE, SD_DATA_OBJ_SIZE);
Please put the variable declarations on separate lines, it's very easy to miss "idx". > + int ret = 1; > + > + for (idx = start; idx <= end; idx++) { Should this be idx < end? Otherwise you are checking one beyond the last SD_DATA_OBJ_SIZE. > + if (inode->data_vdi_id[idx] == 0) { > + break; > + } > + } > + if (idx == start) { > + /* Get te longest length of unallocated sectors */ s/te/the/ > + ret = 0; > + for (idx = start + 1; idx <= end; idx++) { > + if (inode->data_vdi_id[idx] != 0) { > + break; > + } > + } > + } Here is a more concise way of implementing these two loops: int ret = !!inode->data_vdi_id[idx]; for (idx = start + 1; idx < end; idx++) { if (!!inode->data_vdi_id[idx] != ret) { break; } } I like this better because it avoids code duplication, but it's a question of style. Feel free to stick to your approach if you like.