From mboxrd@z Thu Jan  1 00:00:00 1970
Received: from eggs.gnu.org ([2001:4830:134:3::10]:60760)
	by lists.gnu.org with esmtp (Exim 4.71)
	(envelope-from <paolo.bonzini@gmail.com>) id 1ZugXk-00049k-TM
	for qemu-devel@nongnu.org; Fri, 06 Nov 2015 07:55:01 -0500
Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71)
	(envelope-from <paolo.bonzini@gmail.com>) id 1ZugXk-0002xk-30
	for qemu-devel@nongnu.org; Fri, 06 Nov 2015 07:55:00 -0500
Sender: Paolo Bonzini <paolo.bonzini@gmail.com>
References: <1446771897-14628-1-git-send-email-arei.gonglei@huawei.com>
	<20151106103555.GC12285@stefanha-x1.localdomain>
From: Paolo Bonzini <pbonzini@redhat.com>
Message-ID: <563CA317.6080906@redhat.com>
Date: Fri, 6 Nov 2015 13:54:47 +0100
MIME-Version: 1.0
In-Reply-To: <20151106103555.GC12285@stefanha-x1.localdomain>
Content-Type: text/plain; charset=windows-1252
Content-Transfer-Encoding: 8bit
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: Stefan Hajnoczi <stefanha@redhat.com>, arei.gonglei@huawei.com
Cc: kwolf@redhat.com, qemu-devel@nongnu.org, qemu-block@nongnu.org, mst@redhat.com



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.

Thanks,

Paolo