On 3 December 2015 at 16:01, Bill Fischofer <bill.fischo...@linaro.org> wrote:
> > > On Thu, Dec 3, 2015 at 8:30 AM, Zoltan Kiss <zoltan.k...@linaro.org> > 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 <nmo...@kalray.eu> >>> Reviewed-by: Petri Savolainen <petri.savolai...@nokia.com> >>> --- >>> 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, > "We don't actually make use of that RC for now" I don't understand this sentence. > so perhaps the change should be to say the RC is 1..num rather than 0..num. > Perhaps no known implementation returns 0 buffers but that doesn't mean we can go and change the specification. Applications should expect and handle odp_packet_alloc_multi() returning 0 and not (by default) treat this as a fatal error. > >> >> + */ >>> +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 >> lng-odp@lists.linaro.org >> https://lists.linaro.org/mailman/listinfo/lng-odp >> > > > _______________________________________________ > lng-odp mailing list > lng-odp@lists.linaro.org > https://lists.linaro.org/mailman/listinfo/lng-odp > >
_______________________________________________ lng-odp mailing list lng-odp@lists.linaro.org https://lists.linaro.org/mailman/listinfo/lng-odp
