On 04/22/2013 08:00 PM, Stefan Hajnoczi wrote: > 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".
Okay. > >> + 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. No. the end is index of last object, not index of last object + 1. > >> + 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. The trick of your code looks fantastic to me and I like your idea to reduce the duplicated code as much as possible but the sacrifice of code readability for the resulted code is somewhat too high, so I think not worth of it and I'll stick to my stupid but more clear version in V3. Thanks, Yuan