On 11/12/2014 14:39, Yann Droneaud wrote:
> Le jeudi 11 décembre 2014 à 13:09 +0200, Haggai Eran a écrit :
>> On 10/12/2014 18:22, Yann Droneaud wrote:
>>> Hi,
>>>
>>> Le mardi 11 novembre 2014 à 18:36 +0200, Haggai Eran a écrit :
>>>> +{
>>>> + size_t end = offset + length;
>>>> +
>>>> + if (offset > umem->length || end > umem->length || end < offset) {
>>>> + pr_err("ib_umem_copy_from not in range. offset: %zd umem
>>>> length: %zd end: %zd\n",
>>>> + offset, umem->length, end);
>>>> + return -EINVAL;
>>>> + }
>>>> +
>
> I think the test could be rewritten as:
>
> if (offset > umem->length || length > umem->length - offset)
>
> That's one operation less.
>
Okay.
>
>>>> + return sg_pcopy_to_buffer(umem->sg_head.sgl, umem->nmap, dst, length,
>>>> + offset + ib_umem_offset(umem));
>>>> +}
>>>> +EXPORT_SYMBOL(ib_umem_copy_from);
>>>
>>> As the function return a "int", no more than INT_MAX bytes (likely 2^31
>>> - 1) can be copied. Perhaps changing the return type to to ssize_t would
>>> be better (and a check to enfore ssize_t maximum value). Or change the
>>> function could return 0 in case of success or a error code, just like
>>> ib_copy_from_udata().
>>>
>>
>> Okay. I'll change it to match ib_copy_from_udata. We're checking the
>> umem size in the call site of this function anyway, and the only reason
>> I see sg_pcopy_to_buffer would return less than *length* bytes is when
>> reaching the end of the scatterlist.
>>
>
> As the length is compared against umem->length (+ offset), this would
> mean umem->length is not "synchronized" with the length of the data
> described by the scatter/gather list ?
Yes, I don't think this can happen, so we can just return 0 in case of
success like you suggested.
>
> BTW, ib_copy_from_udata() is defined as an inline function. Would it be
> better to have ib_umem_copy_from() being an inline function too ?
> (In such case, I would remove the error message to not duplicate it
> across all modules using the function)
I don't see a great benefit from inlining here. The time to perform this
function is mostly due to the traversal of the scatterlist, and I don't
think an additional function call would make much of a difference.
Regards,
Haggai
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
