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
