| Message ID | 55269432.5090609@linaro.org |
|---|---|
| State | New |
| Headers | show |
I'm not sure what the benefit of the proposed hack would be, since you're returning an opaque value. The error flags are intentionally vague at this point because we haven't defined any standard error categories. As we do, the summary will be augmented with specific access functions for the defined categories. All part of the API evolution. On Thu, Apr 9, 2015 at 10:01 AM, Zoltan Kiss <zoltan.kiss@linaro.org> wrote: > We can say then: > > * @retval non-zero packet has errors, implementation can optionally return > a non-zero error code, STRICTLY FOR DEBUGGING PURPOSES > > I think that would clarify that you shouldn't use this non-zero value to > anything else than saving it into your logs. But any ideas to make it more > clear are welcome. > I came accross this when I found that all my DPDK packets are dropped due > to an error. Currently I have this hack in place to get the error value in > if there is anything wrong: > > diff --git a/platform/linux-generic/odp_packet_flags.c > b/platform/linux-generic/odp_packet_flags.c > index ab3d12f..439255a 100644 > --- a/platform/linux-generic/odp_packet_flags.c > +++ b/platform/linux-generic/odp_packet_flags.c > @@ -10,7 +10,7 @@ > > int odp_packet_has_error(odp_packet_t pkt) > { > - return (odp_packet_hdr(pkt)->error_flags.all != 0); > + return (odp_packet_hdr(pkt)->error_flags.all); > } > > /* Get Input Flags */ > > I think it would be useful for an app developer to have the chance to see > the underlying error code. Otherwise hacks like mine above would born. > > Zoli > > On 08/04/15 18:18, Bill Fischofer wrote: > >> The only problem with documenting the return as !0 is that that syntax >> is ambiguous. The C expression !0 evaluates to a specific value, but >> we're saying that any non-zero value is regarded as true, which is why >> it is documented as such. >> >> On Wed, Apr 8, 2015 at 12:12 PM, Mike Holmes <mike.holmes@linaro.org >> <mailto:mike.holmes@linaro.org>> wrote: >> >> >> >> On 8 April 2015 at 13:10, Bill Fischofer <bill.fischofer@linaro.org >> <mailto:bill.fischofer@linaro.org>> wrote: >> >> These are booleans. However we decided that ODP considers any >> non-zero value to be "true", hence the doc change. The >> linux-generic implementation always returns 1 for true and since >> 1 is a non-zero value that's completely conforming. >> >> >> Agree - it is the optional addition info aspect of !0 I thought >> might be an issue, if two states are returned I have no qualms about >> portability >> >> >> On Wed, Apr 8, 2015 at 12:07 PM, Mike Holmes >> <mike.holmes@linaro.org <mailto:mike.holmes@linaro.org>> wrote: >> >> >> >> On 8 April 2015 at 12:45, Zoltan Kiss >> <zoltan.kiss@linaro.org <mailto:zoltan.kiss@linaro.org>> >> wrote: >> >> Hi, >> >> Now it looks like: >> >> * @retval 1 packet has errors >> * @retval 0 packet has no errors >> >> I found it is better for debugging if it actually >> returns some error code. How about changing it to: >> >> * @retval 0 packet has no errors >> * @retval !0 packet has errors, implementation can >> optionally return an error code >> >> >> This looks like it was intended to be a boolean test so >> extra info might be misleading, and to be a standard people >> can rely on optional is never a good thing. >> >> Is there a minimum set of errors this api can be said will >> return an error code for ? If there is no standard set at >> all I question having anything optional becasue the api will >> have no portability at all. >> >> Taken from the hdr in question it looks like it would be >> this list >> >> struct { >> /* Bitfield flags for each detected error */ >> uint32_t app_error:1; /**< Error bit for application use */ >> uint32_t frame_len:1; /**< Frame length error */ >> uint32_t snap_len:1; /**< Snap length error */ >> uint32_t l2_chksum:1; /**< L2 checksum error, checks TBD */ >> uint32_t ip_err:1; /**< IP error, checks TBD */ >> uint32_t tcp_err:1; /**< TCP error, checks TBD */ >> uint32_t udp_err:1; /**< UDP error, checks TBD */ >> }; >> } error_flags_t; >> >> >> >> Zoli >> _________________________________________________ >> lng-odp mailing list >> lng-odp@lists.linaro.org <mailto:lng-odp@lists.linaro.org >> > >> https://lists.linaro.org/__mailman/listinfo/lng-odp >> <https://lists.linaro.org/mailman/listinfo/lng-odp> >> >> >> >> >> -- >> Mike Holmes >> Technical Manager - Linaro Networking Group >> Linaro.org <http://www.linaro.org/>***│ *Open source >> software for ARM SoCs >> >> __ >> >> >> >> _______________________________________________ >> lng-odp mailing list >> lng-odp@lists.linaro.org <mailto:lng-odp@lists.linaro.org> >> https://lists.linaro.org/mailman/listinfo/lng-odp >> >> >> >> >> >> -- >> Mike Holmes >> Technical Manager - Linaro Networking Group >> Linaro.org <http://www.linaro.org/>***│ *Open source software for >> ARM SoCs >> >> __ >> >> >> >>
On 09/04/15 16:11, Bill Fischofer wrote: > I'm not sure what the benefit of the proposed hack would be, since > you're returning an opaque value. Still, your app can print that number into the log, so if the developer looks up the particular platform implementation, he can get an idea what the problem might be. Or not, if the platform implementation decides to just return a static value (e.g. 1). Still the chance would be given. Of course if we decide later on to return standard error values here, that door would be still open. The error flags are intentionally > vague at this point because we haven't defined any standard error > categories. As we do, the summary will be augmented with specific > access functions for the defined categories. All part of the API evolution. > > On Thu, Apr 9, 2015 at 10:01 AM, Zoltan Kiss <zoltan.kiss@linaro.org > <mailto:zoltan.kiss@linaro.org>> wrote: > > We can say then: > > * @retval non-zero packet has errors, implementation can optionally > return a non-zero error code, STRICTLY FOR DEBUGGING PURPOSES > > I think that would clarify that you shouldn't use this non-zero > value to anything else than saving it into your logs. But any ideas > to make it more clear are welcome. > I came accross this when I found that all my DPDK packets are > dropped due to an error. Currently I have this hack in place to get > the error value in if there is anything wrong: > > diff --git a/platform/linux-generic/odp___packet_flags.c > b/platform/linux-generic/odp___packet_flags.c > index ab3d12f..439255a 100644 > --- a/platform/linux-generic/odp___packet_flags.c > +++ b/platform/linux-generic/odp___packet_flags.c > @@ -10,7 +10,7 @@ > > int odp_packet_has_error(odp___packet_t pkt) > { > - return (odp_packet_hdr(pkt)->error___flags.all != 0); > + return (odp_packet_hdr(pkt)->error___flags.all); > } > > /* Get Input Flags */ > > I think it would be useful for an app developer to have the chance > to see the underlying error code. Otherwise hacks like mine above > would born. > > Zoli > > On 08/04/15 18:18, Bill Fischofer wrote: > > The only problem with documenting the return as !0 is that that > syntax > is ambiguous. The C expression !0 evaluates to a specific > value, but > we're saying that any non-zero value is regarded as true, which > is why > it is documented as such. > > On Wed, Apr 8, 2015 at 12:12 PM, Mike Holmes > <mike.holmes@linaro.org <mailto:mike.holmes@linaro.org> > <mailto:mike.holmes@linaro.org > <mailto:mike.holmes@linaro.org>__>> wrote: > > > > On 8 April 2015 at 13:10, Bill Fischofer > <bill.fischofer@linaro.org <mailto:bill.fischofer@linaro.org> > <mailto:bill.fischofer@linaro.__org > <mailto:bill.fischofer@linaro.org>>> wrote: > > These are booleans. However we decided that ODP > considers any > non-zero value to be "true", hence the doc change. The > linux-generic implementation always returns 1 for true > and since > 1 is a non-zero value that's completely conforming. > > > Agree - it is the optional addition info aspect of !0 I > thought > might be an issue, if two states are returned I have no > qualms about > portability > > > On Wed, Apr 8, 2015 at 12:07 PM, Mike Holmes > <mike.holmes@linaro.org <mailto:mike.holmes@linaro.org> > <mailto:mike.holmes@linaro.org > <mailto:mike.holmes@linaro.org>__>> wrote: > > > > On 8 April 2015 at 12:45, Zoltan Kiss > <zoltan.kiss@linaro.org > <mailto:zoltan.kiss@linaro.org> <mailto:zoltan.kiss@linaro.org > <mailto:zoltan.kiss@linaro.org>__>> wrote: > > Hi, > > Now it looks like: > > * @retval 1 packet has errors > * @retval 0 packet has no errors > > I found it is better for debugging if it actually > returns some error code. How about changing it to: > > * @retval 0 packet has no errors > * @retval !0 packet has errors, > implementation can > optionally return an error code > > > This looks like it was intended to be a boolean test so > extra info might be misleading, and to be a > standard people > can rely on optional is never a good thing. > > Is there a minimum set of errors this api can be > said will > return an error code for ? If there is no standard > set at > all I question having anything optional becasue the > api will > have no portability at all. > > Taken from the hdr in question it looks like it > would be > this list > > struct { > /* Bitfield flags for each detected error */ > uint32_t app_error:1; /**< Error bit for > application use */ > uint32_t frame_len:1; /**< Frame length error */ > uint32_t snap_len:1; /**< Snap length error */ > uint32_t l2_chksum:1; /**< L2 checksum error, > checks TBD */ > uint32_t ip_err:1; /**< IP error, checks TBD */ > uint32_t tcp_err:1; /**< TCP error, checks TBD */ > uint32_t udp_err:1; /**< UDP error, checks TBD */ > }; > } error_flags_t; > > > > Zoli > ___________________________________________________ > lng-odp mailing list > lng-odp@lists.linaro.org <mailto:lng-odp@lists.linaro.org> > <mailto:lng-odp@lists.linaro.__org > <mailto:lng-odp@lists.linaro.org>> > https://lists.linaro.org/____mailman/listinfo/lng-odp > <https://lists.linaro.org/__mailman/listinfo/lng-odp> > > <https://lists.linaro.org/__mailman/listinfo/lng-odp > <https://lists.linaro.org/mailman/listinfo/lng-odp>> > > > > > -- > Mike Holmes > Technical Manager - Linaro Networking Group > Linaro.org <http://www.linaro.org/>***│ *Open source > software for ARM SoCs > > __ > > > > _________________________________________________ > lng-odp mailing list > lng-odp@lists.linaro.org <mailto:lng-odp@lists.linaro.org> > <mailto:lng-odp@lists.linaro.__org > <mailto:lng-odp@lists.linaro.org>> > https://lists.linaro.org/__mailman/listinfo/lng-odp > <https://lists.linaro.org/mailman/listinfo/lng-odp> > > > > > > -- > Mike Holmes > Technical Manager - Linaro Networking Group > Linaro.org <http://www.linaro.org/>***│ *Open source > software for > ARM SoCs > > __ > > > >
I agree with the comment that ODP should solve actual implementer need or it will writhe with local hacks to make it usable. Do we need to get some of access methods defined as you mention Bill, because I also think we should not expose the underlying structure unless we are sure it will be part of the API definitions. A proposal for odp-next to solve the issue could be in for 1.1 at the end of the month. A side thought, could those additional codes be in odp_errno <http://docs.opendataplane.org/linux-generic-doxygen-html/api_guide_lines.html#errno> it may not be quite as convenient, but is described in the ODP framework as a possible solution to this issue. They are then documented in the API - just as Posix does. I feel that may be misleading in this case though since it is a packet error, not a sw error. On 9 April 2015 at 11:11, Bill Fischofer <bill.fischofer@linaro.org> wrote: > I'm not sure what the benefit of the proposed hack would be, since you're > returning an opaque value. The error flags are intentionally vague at this > point because we haven't defined any standard error categories. As we do, > the summary will be augmented with specific access functions for the > defined categories. All part of the API evolution. > > On Thu, Apr 9, 2015 at 10:01 AM, Zoltan Kiss <zoltan.kiss@linaro.org> > wrote: > >> We can say then: >> >> * @retval non-zero packet has errors, implementation can optionally >> return a non-zero error code, STRICTLY FOR DEBUGGING PURPOSES >> >> I think that would clarify that you shouldn't use this non-zero value to >> anything else than saving it into your logs. But any ideas to make it more >> clear are welcome. >> I came accross this when I found that all my DPDK packets are dropped due >> to an error. Currently I have this hack in place to get the error value in >> if there is anything wrong: >> >> diff --git a/platform/linux-generic/odp_packet_flags.c >> b/platform/linux-generic/odp_packet_flags.c >> index ab3d12f..439255a 100644 >> --- a/platform/linux-generic/odp_packet_flags.c >> +++ b/platform/linux-generic/odp_packet_flags.c >> @@ -10,7 +10,7 @@ >> >> int odp_packet_has_error(odp_packet_t pkt) >> { >> - return (odp_packet_hdr(pkt)->error_flags.all != 0); >> + return (odp_packet_hdr(pkt)->error_flags.all); >> } >> >> /* Get Input Flags */ >> >> I think it would be useful for an app developer to have the chance to see >> the underlying error code. Otherwise hacks like mine above would born. >> >> Zoli >> >> On 08/04/15 18:18, Bill Fischofer wrote: >> >>> The only problem with documenting the return as !0 is that that syntax >>> is ambiguous. The C expression !0 evaluates to a specific value, but >>> we're saying that any non-zero value is regarded as true, which is why >>> it is documented as such. >>> >>> On Wed, Apr 8, 2015 at 12:12 PM, Mike Holmes <mike.holmes@linaro.org >>> <mailto:mike.holmes@linaro.org>> wrote: >>> >>> >>> >>> On 8 April 2015 at 13:10, Bill Fischofer <bill.fischofer@linaro.org >>> <mailto:bill.fischofer@linaro.org>> wrote: >>> >>> These are booleans. However we decided that ODP considers any >>> non-zero value to be "true", hence the doc change. The >>> linux-generic implementation always returns 1 for true and since >>> 1 is a non-zero value that's completely conforming. >>> >>> >>> Agree - it is the optional addition info aspect of !0 I thought >>> might be an issue, if two states are returned I have no qualms about >>> portability >>> >>> >>> On Wed, Apr 8, 2015 at 12:07 PM, Mike Holmes >>> <mike.holmes@linaro.org <mailto:mike.holmes@linaro.org>> wrote: >>> >>> >>> >>> On 8 April 2015 at 12:45, Zoltan Kiss >>> <zoltan.kiss@linaro.org <mailto:zoltan.kiss@linaro.org>> >>> wrote: >>> >>> Hi, >>> >>> Now it looks like: >>> >>> * @retval 1 packet has errors >>> * @retval 0 packet has no errors >>> >>> I found it is better for debugging if it actually >>> returns some error code. How about changing it to: >>> >>> * @retval 0 packet has no errors >>> * @retval !0 packet has errors, implementation can >>> optionally return an error code >>> >>> >>> This looks like it was intended to be a boolean test so >>> extra info might be misleading, and to be a standard people >>> can rely on optional is never a good thing. >>> >>> Is there a minimum set of errors this api can be said will >>> return an error code for ? If there is no standard set at >>> all I question having anything optional becasue the api will >>> have no portability at all. >>> >>> Taken from the hdr in question it looks like it would be >>> this list >>> >>> struct { >>> /* Bitfield flags for each detected error */ >>> uint32_t app_error:1; /**< Error bit for application use */ >>> uint32_t frame_len:1; /**< Frame length error */ >>> uint32_t snap_len:1; /**< Snap length error */ >>> uint32_t l2_chksum:1; /**< L2 checksum error, checks TBD */ >>> uint32_t ip_err:1; /**< IP error, checks TBD */ >>> uint32_t tcp_err:1; /**< TCP error, checks TBD */ >>> uint32_t udp_err:1; /**< UDP error, checks TBD */ >>> }; >>> } error_flags_t; >>> >>> >>> >>> Zoli >>> _________________________________________________ >>> lng-odp mailing list >>> lng-odp@lists.linaro.org <mailto:lng-odp@lists.linaro. >>> org> >>> https://lists.linaro.org/__mailman/listinfo/lng-odp >>> <https://lists.linaro.org/mailman/listinfo/lng-odp> >>> >>> >>> >>> >>> -- >>> Mike Holmes >>> Technical Manager - Linaro Networking Group >>> Linaro.org <http://www.linaro.org/>***│ *Open source >>> software for ARM SoCs >>> >>> __ >>> >>> >>> >>> _______________________________________________ >>> lng-odp mailing list >>> lng-odp@lists.linaro.org <mailto:lng-odp@lists.linaro.org> >>> https://lists.linaro.org/mailman/listinfo/lng-odp >>> >>> >>> >>> >>> >>> -- >>> Mike Holmes >>> Technical Manager - Linaro Networking Group >>> Linaro.org <http://www.linaro.org/>***│ *Open source software for >>> ARM SoCs >>> >>> __ >>> >>> >>> >>> >
diff --git a/platform/linux-generic/odp_packet_flags.c b/platform/linux-generic/odp_packet_flags.c index ab3d12f..439255a 100644 --- a/platform/linux-generic/odp_packet_flags.c +++ b/platform/linux-generic/odp_packet_flags.c @@ -10,7 +10,7 @@ int odp_packet_has_error(odp_packet_t pkt) { - return (odp_packet_hdr(pkt)->error_flags.all != 0); + return (odp_packet_hdr(pkt)->error_flags.all); } /* Get Input Flags */