From mboxrd@z Thu Jan  1 00:00:00 1970
Received: from eggs.gnu.org ([208.118.235.92]:49522)
	by lists.gnu.org with esmtp (Exim 4.71)
	(envelope-from <namei.unix@gmail.com>) id 1UUFaS-0007KL-3x
	for qemu-devel@nongnu.org; Mon, 22 Apr 2013 08:11:16 -0400
Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71)
	(envelope-from <namei.unix@gmail.com>) id 1UUFaM-0007g4-8J
	for qemu-devel@nongnu.org; Mon, 22 Apr 2013 08:11:12 -0400
Received: from mail-pd0-f177.google.com ([209.85.192.177]:56954)
	by eggs.gnu.org with esmtp (Exim 4.71)
	(envelope-from <namei.unix@gmail.com>) id 1UUFaM-0007fg-27
	for qemu-devel@nongnu.org; Mon, 22 Apr 2013 08:11:06 -0400
Received: by mail-pd0-f177.google.com with SMTP id p11so226250pdj.36
	for <qemu-devel@nongnu.org>; Mon, 22 Apr 2013 05:11:05 -0700 (PDT)
Message-ID: <517528D2.1040409@gmail.com>
Date: Mon, 22 Apr 2013 20:10:58 +0800
From: Liu Yuan <namei.unix@gmail.com>
MIME-Version: 1.0
References: <1366613950-10918-1-git-send-email-namei.unix@gmail.com>
	<1366613950-10918-3-git-send-email-namei.unix@gmail.com>
	<20130422120007.GB21317@stefanha-thinkpad.redhat.com>
In-Reply-To: <20130422120007.GB21317@stefanha-thinkpad.redhat.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
Subject: Re: [Qemu-devel] [PATCH v2 2/2] sheepdog: implement
	.bdrv_co_is_allocated()
List-Id: <qemu-devel.nongnu.org>
List-Unsubscribe: <https://lists.nongnu.org/mailman/options/qemu-devel>,
	<mailto:qemu-devel-request@nongnu.org?subject=unsubscribe>
List-Archive: <http://lists.nongnu.org/archive/html/qemu-devel>
List-Post: <mailto:qemu-devel@nongnu.org>
List-Help: <mailto:qemu-devel-request@nongnu.org?subject=help>
List-Subscribe: <https://lists.nongnu.org/mailman/listinfo/qemu-devel>,
	<mailto:qemu-devel-request@nongnu.org?subject=subscribe>
To: Stefan Hajnoczi <stefanha@gmail.com>
Cc: Kevin Wolf <kwolf@redhat.com>, MORITA Kazutaka <morita.kazutaka@lab.ntt.co.jp>, sheepdog@lists.wpkg.org, qemu-devel@nongnu.org, Stefan Hajnoczi <stefanha@redhat.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