Re: [Qemu-devel] [PATCH] virtio-blk: trivial code optimization

[Gonglei <arei.gonglei@huawei.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