On Thu, Dec 3, 2015 at 8:30 AM, Zoltan Kiss <[email protected]> wrote:
> Hi, > > I know it's a late cry, but I've found two problems while implementing > this for ODP-DPDK, and the apply for odp_packet_alloc_multi() as well. See > inline: > > On 28/10/15 15:31, Nicolas Morey-Chaisemartin wrote: > >> Signed-off-by: Nicolas Morey-Chaisemartin <[email protected]> >> Reviewed-by: Petri Savolainen <[email protected]> >> --- >> include/odp/api/buffer.h | 26 ++++++++++++++++++++++++++ >> 1 file changed, 26 insertions(+) >> >> diff --git a/include/odp/api/buffer.h b/include/odp/api/buffer.h >> index aea273f..6631f47 100644 >> --- a/include/odp/api/buffer.h >> +++ b/include/odp/api/buffer.h >> @@ -105,6 +105,20 @@ odp_pool_t odp_buffer_pool(odp_buffer_t buf); >> odp_buffer_t odp_buffer_alloc(odp_pool_t pool); >> >> /** >> + * Allocate multiple buffers >> + >> + * Otherwise like odp_buffer_alloc(), but allocates multiple buffers >> from a pool >> + * >> + * @param pool Pool handle >> + * @param[out] buf Array of buffer handles for output >> + * @param num Maximum number of buffers to allocate >> + * >> + * @return Number of buffers actually allocated (0 ... num) >> + * @retval <0 on failure >> > > These two lines contradicts each other: the first says we should return > how much we managed to allocate, and we will always allocate at least 0 > buffers. > The second says in case of error a negative value, but even if we fail > straight away, the first line commands us to return 0. Moreover, if we > managed to allocate at least some of the buffers and then we return a > negative value due to some error (e.g. ENOMEM, or anything else), the > application won't know if there is any buffer to use in the array. Either > we should clarify that the platform should release them in case of an > error, or (and I think that would be the better thing to do) we should > return how much we allocated. > Altogether, I think we should remove the "<0 on failure" statement and set > the errno instead. If you agree, Nicolas, would you submit a patch for that? This is just being consistent with other multi() APIs like odp_queue_enq_multi(). I believe we had this discussion surrounding those APIs and decided to keep these semantics. Negative RCs say the error is permanent. An RC of 0 would indicate that nothing was done however the operation is retryable. We don't actually make use of that RC for now, so perhaps the change should be to say the RC is 1..num rather than 0..num. > > + */ >> +int odp_buffer_alloc_multi(odp_pool_t pool, odp_buffer_t buf[], int num); >> > > 'num' should be unsigned, a negative number doesn't make sense in this > case. Although changing this breaks the ABI, so I'm not sure if we should > fix this or just leave it. Again this is copied directly from the other multi() APIs. Agree they probably should be unsigned but I don't think it's a big deal since in the unlikely event a negative number was supplied you wouldn't get any buffers anyway. By the same argument specifying 0 here wouldn't make sense semantically and yet that's a valid int and unsigned. > > > _______________________________________________ > lng-odp mailing list > [email protected] > https://lists.linaro.org/mailman/listinfo/lng-odp >
_______________________________________________ lng-odp mailing list [email protected] https://lists.linaro.org/mailman/listinfo/lng-odp
