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

[Liu Yuan <namei.unix@gmail.com>]

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