Was there anything you want to change in this patch? The only thing here is that we haven't really defined what "valid" means.
/** * Tests if buffer is valid * * @param buf Buffer handle (possibly invalid) * @note This is the only buffer API function which accepts invalid buffer * handles (any bit value) without causing undefined behavior. * * @retval 1 Buffer handle represents a valid buffer. * @retval 0 Buffer handle does not represent a valid buffer. */ int odp_buffer_is_valid(odp_buffer_t buf); On 4 February 2015 at 13:42, Ola Liljedahl <[email protected]> wrote: > On 4 February 2015 at 13:28, Savolainen, Petri (NSN - FI/Espoo) > <[email protected]> wrote: >> >> >>> -----Original Message----- >>> From: ext Ola Liljedahl [mailto:[email protected]] >>> Sent: Wednesday, February 04, 2015 12:01 PM >>> To: Savolainen, Petri (NSN - FI/Espoo) >>> Cc: [email protected] >>> Subject: Re: [lng-odp] [PATCHv5 05/18] api: odp_buffer.h: undefined >>> behavior description >>> >>> On 4 February 2015 at 10:51, Savolainen, Petri (NSN - FI/Espoo) >>> <[email protected]> wrote: >>> > >>> > >>> >> -----Original Message----- >>> >> From: [email protected] [mailto:lng-odp- >>> >> [email protected]] On Behalf Of ext Ola Liljedahl >>> >> Sent: Tuesday, February 03, 2015 6:48 PM >>> >> To: [email protected] >>> >> Subject: [lng-odp] [PATCHv5 05/18] api: odp_buffer.h: undefined >>> behavior >>> >> description >>> >> >>> >> Documented API calls which are guaranteed to handle invalid/stale >>> handles. >>> >> >>> >> Signed-off-by: Ola Liljedahl <[email protected]> >>> >> --- >>> >> (This document/code contribution attached is provided under the terms >>> of >>> >> agreement LES-LTM-21309) >>> >> >>> >> include/odp/api/buffer.h | 4 +++- >>> >> 1 file changed, 3 insertions(+), 1 deletion(-) >>> >> >>> >> diff --git a/include/odp/api/buffer.h b/include/odp/api/buffer.h >>> >> index 12b2f5a..1eb2e5a 100644 >>> >> --- a/include/odp/api/buffer.h >>> >> +++ b/include/odp/api/buffer.h >>> >> @@ -88,7 +88,9 @@ uint32_t odp_buffer_size(odp_buffer_t buf); >>> >> /** >>> >> * Tests if buffer is valid >>> >> * >>> >> - * @param buf Buffer handle >>> >> + * @param buf Buffer handle (possibly invalid) >>> >> + * @note This is the only buffer API function which accepts invalid >>> >> buffer >>> >> + * handles (any bit value) without causing undefined behavior. >>> >> * >>> >> * @retval 1 Buffer handle represents a valid buffer. >>> >> * @retval 0 Buffer handle does not represent a valid buffer. >>> >> -- >>> > >>> > Originally, this and odp_packet_is_valid() were defined to enable full >>> sanity check over the object. During debugging user could insert these on >>> the way and (hopefully) catch the point where e.g. packet metadata (e.g. >>> segment linking) was corrupted. >>> Should we specify this "full sanity check of object" as well? That >>> part is not obvious now and an ODP implementer could easily interpret >>> "represents a valid buffer" in a less complete way. >>> Perhaps this function needs more return values. E.g. >>> @retval 0 Buffer handle is invalid (no further checks done) >>> @retval 1 Buffer handle is valid but buffer is corrupted >>> @retval 2 Buffer handle is valid and buffer seems correct >> >> Another option is to set errno to indicate the reason of failure. Valid vs. >> broken buffer/packet is the most valuable information from the call. It's >> secondary to know why it's broken. No need to change return values or add >> errno for v1.0. >> >> Also if this function needs to accept bad handles, I think it means that in >> general the handle structure/mapping to memory/etc have to be error >> tolerant, which we try to not force on other calls (by specifying undefined >> behavior on bad handles). To me it sounds all or nothing - either all buffer >> calls (that access only odp_buffer_t, no shared resources) have undefined >> behavior or non. >> >> So, I'd leave the patch out from the set, and improve documentation (add >> "sanity check") with another patch. > Agreed. The problem is still not well understood. But the current > function definition is weak and liable to be misunderstood. > >> >>> >>> > >>> > So, I think this function could still crash on bad handles. Is there a >>> use case that application would just want to check the handle validity >>> (without crashing), but not the object sanity? >>> I was thinking that some post-mortem dump code could iterate over all >>> known (to the application) buffer and packet handles and save >>> information about those. But in a crash situation, you can't really be >>> sure that the handles you find are correct, e.g. could be stale >>> handles so treated as invalid by an implementation. But you don't want >>> to crash in your PMD code, in this case just save that this specific >>> handle is invalid. >> >> Good point, but maybe post mortem tools are a separate topic (vendor >> specific). Can we standardize what happens / can be done after crash? I >> guess not. > In my view, this post-mortem dumping is the responsibility of the > application. ODP just needs to guarantee that I can (attempt to) > inspect various ODP objects without crashing. Invalid handles, corrupt > data structures etc should be reported back to the caller, not invoke > undefined behavior. > > > >> >> -Petri >> >>> >>> > >>> > -Petri >>> > >>> > _______________________________________________ lng-odp mailing list [email protected] http://lists.linaro.org/mailman/listinfo/lng-odp
