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

[Stefan Hajnoczi]

On Mon, Apr 22, 2013 at 08:10:58PM +0800, Liu Yuan wrote:
> 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.

Imagine sector_num = 0 and nb_sectors = SD_DATA_OBJ_SIZE / BDRV_SECTOR_SIZE.

start = 0
end = 1

You don't want object 1, only object 0.