From mboxrd@z Thu Jan  1 00:00:00 1970
Received: from eggs.gnu.org ([2001:4830:134:3::10]:36257)
	by lists.gnu.org with esmtp (Exim 4.71)
	(envelope-from <arei.gonglei@huawei.com>) id 1Zvbso-0000Dw-A9
	for qemu-devel@nongnu.org; Sun, 08 Nov 2015 21:08:35 -0500
Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71)
	(envelope-from <arei.gonglei@huawei.com>) id 1Zvbsk-0003QW-8J
	for qemu-devel@nongnu.org; Sun, 08 Nov 2015 21:08:34 -0500
Message-ID: <56400002.9010708@huawei.com>
Date: Mon, 9 Nov 2015 10:08:02 +0800
From: Gonglei <arei.gonglei@huawei.com>
MIME-Version: 1.0
References: <1446771897-14628-1-git-send-email-arei.gonglei@huawei.com>
	<20151106103555.GC12285@stefanha-x1.localdomain>
	<563CA317.6080906@redhat.com>
In-Reply-To: <563CA317.6080906@redhat.com>
Content-Type: text/plain; charset="windows-1252"
Content-Transfer-Encoding: 7bit
Subject: Re: [Qemu-devel] [PATCH] virtio-blk: trivial code optimization
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: Paolo Bonzini <pbonzini@redhat.com>, Stefan Hajnoczi <stefanha@redhat.com>
Cc: kwolf@redhat.com, qemu-devel@nongnu.org, qemu-block@nongnu.org, mst@redhat.com

On 2015/11/6 20:54, Paolo Bonzini wrote:
> 
> 
> On 06/11/2015 11:35, Stefan Hajnoczi wrote:
>>>>              if (niov + req->qiov.niov > IOV_MAX) {
>>>>                  merge = false;
>>>> +                goto unmerge;
>>>>              }
>>>>  
>>>>              /* merge would exceed maximum transfer length of backend device */
>>>>              if (req->qiov.size / BDRV_SECTOR_SIZE + nb_sectors > max_xfer_len) {
>>>>                  merge = false;
>>>> +                goto unmerge;
>>>>              }
>>>>  
>>>>              /* requests are not sequential */
>>>>              if (sector_num + nb_sectors != req->sector_num) {
>>>>                  merge = false;
>>>>              }
>>>> -
>>>> +unmerge:
>> C has a way of expressing this without gotos.  Please use else if:
>>
>>   if (a) {
>>       ...
>>   } else if (b) {
>>       ...
>>   } else if (c) {
>>       ...
>>   }
> 
> Another way is
> 
> if (niov + req->qiov.niov > IOV_MAX ||
>     req->qiov.size / BDRV_SECTOR_SIZE + nb_sectors > max_xfer_len ||
>     sector_num + nb_sectors != req->sector_num) {
>     submit_requests(...)
>     ...
> }
> 
> While at it, we could reorder the conditions so that the most common
> ("requests are not sequential") comes first.
> 
> I'm not sure about handling of overflow.  It's probably better to
> write conditions as "new > max - old" (e.g. "niov > IOV_MAX -
> req->qiov.niov") rather than "old + new > max".  The former is always
> safe, because we know that old <= max and there can be no integer
> overflow.
> 
Nice points.  Thanks, both of you.

Regards,
-Gonglei