Re: [Qemu-devel] [PATCH v2 2/2] sheepdog: implement .bdrv_co_is_allocated()

[Stefan Hajnoczi]

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.