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

[Paolo Bonzini <pbonzini@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