| Message ID | 1436183828-7364-1-git-send-email-maxim.uvarov@linaro.org |
|---|---|
| State | New |
| Headers | show |
On Mon, Jul 6, 2015 at 6:57 AM, Maxim Uvarov <maxim.uvarov@linaro.org> wrote: > All examples of odp code do not check odp_schedule() output, > I.e.: > ev = odp_schedule() > pkt = odp_packet_from_event(ev) > > Because of ev can be not only packet (timer, crypto operation), add > check that only packet event can be converted to packet. > > Signed-off-by: Maxim Uvarov <maxim.uvarov@linaro.org> > --- > After Zoltan's changes to API-NEXT I think it's reasonable to add check to > odp_packet_from_event() to make linux-generic catch other non packets > events. > > Thanks, > Maxim. > > platform/linux-generic/odp_crypto.c | 2 +- > platform/linux-generic/odp_packet.c | 3 +++ > 2 files changed, 4 insertions(+), 1 deletion(-) > > diff --git a/platform/linux-generic/odp_crypto.c > b/platform/linux-generic/odp_crypto.c > index d49e256..22071d8 100644 > --- a/platform/linux-generic/odp_crypto.c > +++ b/platform/linux-generic/odp_crypto.c > @@ -40,7 +40,7 @@ static odp_crypto_global_t *global; > static > odp_crypto_generic_op_result_t *get_op_result_from_event(odp_event_t ev) > { > - return &(odp_packet_hdr(odp_packet_from_event(ev))->op_result); > + return &(odp_packet_hdr((odp_packet_t)ev)->op_result); > How is this clearer than the above? The official cast functions should be used here. Isn't that what they're for? > } > > static > diff --git a/platform/linux-generic/odp_packet.c > b/platform/linux-generic/odp_packet.c > index 668ddda..586ad09 100644 > --- a/platform/linux-generic/odp_packet.c > +++ b/platform/linux-generic/odp_packet.c > @@ -77,6 +77,9 @@ odp_buffer_t _odp_packet_to_buffer(odp_packet_t pkt) > > odp_packet_t odp_packet_from_event(odp_event_t ev) > { > + if (odp_unlikely(odp_event_type(ev) != ODP_EVENT_PACKET)) > + ODP_ABORT("Dispatching not packet event %d\n", > + odp_event_type(ev)); > return (odp_packet_t)ev; > } > > -- > 1.9.1 > > _______________________________________________ > lng-odp mailing list > lng-odp@lists.linaro.org > https://lists.linaro.org/mailman/listinfo/lng-odp >
On 07/06/15 14:59, Bill Fischofer wrote: > > > On Mon, Jul 6, 2015 at 6:57 AM, Maxim Uvarov <maxim.uvarov@linaro.org > <mailto:maxim.uvarov@linaro.org>> wrote: > > All examples of odp code do not check odp_schedule() output, > I.e.: > ev = odp_schedule() > pkt = odp_packet_from_event(ev) > > Because of ev can be not only packet (timer, crypto operation), add > check that only packet event can be converted to packet. > > Signed-off-by: Maxim Uvarov <maxim.uvarov@linaro.org > <mailto:maxim.uvarov@linaro.org>> > --- > After Zoltan's changes to API-NEXT I think it's reasonable to add > check to > odp_packet_from_event() to make linux-generic catch other non > packets events. > > Thanks, > Maxim. > > platform/linux-generic/odp_crypto.c | 2 +- > platform/linux-generic/odp_packet.c | 3 +++ > 2 files changed, 4 insertions(+), 1 deletion(-) > > diff --git a/platform/linux-generic/odp_crypto.c > b/platform/linux-generic/odp_crypto.c > index d49e256..22071d8 100644 > --- a/platform/linux-generic/odp_crypto.c > +++ b/platform/linux-generic/odp_crypto.c > @@ -40,7 +40,7 @@ static odp_crypto_global_t *global; > static > odp_crypto_generic_op_result_t > *get_op_result_from_event(odp_event_t ev) > { > - return > &(odp_packet_hdr(odp_packet_from_event(ev))->op_result); > + return &(odp_packet_hdr((odp_packet_t)ev)->op_result); > > > How is this clearer than the above? The official cast functions > should be used here. Isn't that what they're for? This change here because of odp_crypto_operation() changes event type from PACKET to CRYPTO: completion_event = odp_packet_to_event(params->out_pkt); _odp_buffer_event_type_set( odp_buffer_from_event(completion_event), ODP_EVENT_CRYPTO_COMPL); So following odp_packet_from_event() will cast CRYPTO_COMPL back to packet inside get_op_result_from_event() Maxim. > } > > static > diff --git a/platform/linux-generic/odp_packet.c > b/platform/linux-generic/odp_packet.c > index 668ddda..586ad09 100644 > --- a/platform/linux-generic/odp_packet.c > +++ b/platform/linux-generic/odp_packet.c > @@ -77,6 +77,9 @@ odp_buffer_t _odp_packet_to_buffer(odp_packet_t pkt) > > odp_packet_t odp_packet_from_event(odp_event_t ev) > { > + if (odp_unlikely(odp_event_type(ev) != ODP_EVENT_PACKET)) > + ODP_ABORT("Dispatching not packet event %d\n", > + odp_event_type(ev)); > return (odp_packet_t)ev; > } > > -- > 1.9.1 > > _______________________________________________ > lng-odp mailing list > lng-odp@lists.linaro.org <mailto:lng-odp@lists.linaro.org> > https://lists.linaro.org/mailman/listinfo/lng-odp > >
Yes, but it uses an internal API (_odp_buffer_event_type_set()) to do this. On Mon, Jul 6, 2015 at 7:04 AM, Maxim Uvarov <maxim.uvarov@linaro.org> wrote: > On 07/06/15 14:59, Bill Fischofer wrote: > >> >> >> On Mon, Jul 6, 2015 at 6:57 AM, Maxim Uvarov <maxim.uvarov@linaro.org >> <mailto:maxim.uvarov@linaro.org>> wrote: >> >> All examples of odp code do not check odp_schedule() output, >> I.e.: >> ev = odp_schedule() >> pkt = odp_packet_from_event(ev) >> >> Because of ev can be not only packet (timer, crypto operation), add >> check that only packet event can be converted to packet. >> >> Signed-off-by: Maxim Uvarov <maxim.uvarov@linaro.org >> <mailto:maxim.uvarov@linaro.org>> >> --- >> After Zoltan's changes to API-NEXT I think it's reasonable to add >> check to >> odp_packet_from_event() to make linux-generic catch other non >> packets events. >> >> Thanks, >> Maxim. >> >> platform/linux-generic/odp_crypto.c | 2 +- >> platform/linux-generic/odp_packet.c | 3 +++ >> 2 files changed, 4 insertions(+), 1 deletion(-) >> >> diff --git a/platform/linux-generic/odp_crypto.c >> b/platform/linux-generic/odp_crypto.c >> index d49e256..22071d8 100644 >> --- a/platform/linux-generic/odp_crypto.c >> +++ b/platform/linux-generic/odp_crypto.c >> @@ -40,7 +40,7 @@ static odp_crypto_global_t *global; >> static >> odp_crypto_generic_op_result_t >> *get_op_result_from_event(odp_event_t ev) >> { >> - return >> &(odp_packet_hdr(odp_packet_from_event(ev))->op_result); >> + return &(odp_packet_hdr((odp_packet_t)ev)->op_result); >> >> >> How is this clearer than the above? The official cast functions should >> be used here. Isn't that what they're for? >> > This change here because of > odp_crypto_operation() changes event type from PACKET to CRYPTO: > > completion_event = odp_packet_to_event(params->out_pkt); > _odp_buffer_event_type_set( > odp_buffer_from_event(completion_event), > ODP_EVENT_CRYPTO_COMPL); > > So following odp_packet_from_event() will cast CRYPTO_COMPL back to packet > inside get_op_result_from_event() > > Maxim. > > > } >> >> static >> diff --git a/platform/linux-generic/odp_packet.c >> b/platform/linux-generic/odp_packet.c >> index 668ddda..586ad09 100644 >> --- a/platform/linux-generic/odp_packet.c >> +++ b/platform/linux-generic/odp_packet.c >> @@ -77,6 +77,9 @@ odp_buffer_t _odp_packet_to_buffer(odp_packet_t pkt) >> >> odp_packet_t odp_packet_from_event(odp_event_t ev) >> { >> + if (odp_unlikely(odp_event_type(ev) != ODP_EVENT_PACKET)) >> + ODP_ABORT("Dispatching not packet event %d\n", >> + odp_event_type(ev)); >> return (odp_packet_t)ev; >> } >> >> -- >> 1.9.1 >> >> _______________________________________________ >> lng-odp mailing list >> lng-odp@lists.linaro.org <mailto:lng-odp@lists.linaro.org> >> https://lists.linaro.org/mailman/listinfo/lng-odp >> >> >> >
On 07/06/15 15:08, Bill Fischofer wrote: > Yes, but it uses an internal API (_odp_buffer_event_type_set()) to do > this. But I added ABORT() if type is not PACKET. _odp_buffer_event_type_set() switched type to CRYPTO. So ABORT() will trigger. So I updated crypto function to not trap on that ABORT(). Maxim. > > On Mon, Jul 6, 2015 at 7:04 AM, Maxim Uvarov <maxim.uvarov@linaro.org > <mailto:maxim.uvarov@linaro.org>> wrote: > > On 07/06/15 14:59, Bill Fischofer wrote: > > > > On Mon, Jul 6, 2015 at 6:57 AM, Maxim Uvarov > <maxim.uvarov@linaro.org <mailto:maxim.uvarov@linaro.org> > <mailto:maxim.uvarov@linaro.org > <mailto:maxim.uvarov@linaro.org>>> wrote: > > All examples of odp code do not check odp_schedule() output, > I.e.: > ev = odp_schedule() > pkt = odp_packet_from_event(ev) > > Because of ev can be not only packet (timer, crypto > operation), add > check that only packet event can be converted to packet. > > Signed-off-by: Maxim Uvarov <maxim.uvarov@linaro.org > <mailto:maxim.uvarov@linaro.org> > <mailto:maxim.uvarov@linaro.org > <mailto:maxim.uvarov@linaro.org>>> > --- > After Zoltan's changes to API-NEXT I think it's > reasonable to add > check to > odp_packet_from_event() to make linux-generic catch other non > packets events. > > Thanks, > Maxim. > > platform/linux-generic/odp_crypto.c | 2 +- > platform/linux-generic/odp_packet.c | 3 +++ > 2 files changed, 4 insertions(+), 1 deletion(-) > > diff --git a/platform/linux-generic/odp_crypto.c > b/platform/linux-generic/odp_crypto.c > index d49e256..22071d8 100644 > --- a/platform/linux-generic/odp_crypto.c > +++ b/platform/linux-generic/odp_crypto.c > @@ -40,7 +40,7 @@ static odp_crypto_global_t *global; > static > odp_crypto_generic_op_result_t > *get_op_result_from_event(odp_event_t ev) > { > - return > &(odp_packet_hdr(odp_packet_from_event(ev))->op_result); > + return &(odp_packet_hdr((odp_packet_t)ev)->op_result); > > > How is this clearer than the above? The official cast > functions should be used here. Isn't that what they're for? > > This change here because of > odp_crypto_operation() changes event type from PACKET to CRYPTO: > > completion_event = odp_packet_to_event(params->out_pkt); > _odp_buffer_event_type_set( > odp_buffer_from_event(completion_event), > ODP_EVENT_CRYPTO_COMPL); > > So following odp_packet_from_event() will cast CRYPTO_COMPL back > to packet inside get_op_result_from_event() > > Maxim. > > > } > > static > diff --git a/platform/linux-generic/odp_packet.c > b/platform/linux-generic/odp_packet.c > index 668ddda..586ad09 100644 > --- a/platform/linux-generic/odp_packet.c > +++ b/platform/linux-generic/odp_packet.c > @@ -77,6 +77,9 @@ odp_buffer_t > _odp_packet_to_buffer(odp_packet_t pkt) > > odp_packet_t odp_packet_from_event(odp_event_t ev) > { > + if (odp_unlikely(odp_event_type(ev) != > ODP_EVENT_PACKET)) > + ODP_ABORT("Dispatching not packet event %d\n", > + odp_event_type(ev)); > return (odp_packet_t)ev; > } > > -- > 1.9.1 > > _______________________________________________ > 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 > > > >
diff --git a/platform/linux-generic/odp_crypto.c b/platform/linux-generic/odp_crypto.c index d49e256..22071d8 100644 --- a/platform/linux-generic/odp_crypto.c +++ b/platform/linux-generic/odp_crypto.c @@ -40,7 +40,7 @@ static odp_crypto_global_t *global; static odp_crypto_generic_op_result_t *get_op_result_from_event(odp_event_t ev) { - return &(odp_packet_hdr(odp_packet_from_event(ev))->op_result); + return &(odp_packet_hdr((odp_packet_t)ev)->op_result); } static diff --git a/platform/linux-generic/odp_packet.c b/platform/linux-generic/odp_packet.c index 668ddda..586ad09 100644 --- a/platform/linux-generic/odp_packet.c +++ b/platform/linux-generic/odp_packet.c @@ -77,6 +77,9 @@ odp_buffer_t _odp_packet_to_buffer(odp_packet_t pkt) odp_packet_t odp_packet_from_event(odp_event_t ev) { + if (odp_unlikely(odp_event_type(ev) != ODP_EVENT_PACKET)) + ODP_ABORT("Dispatching not packet event %d\n", + odp_event_type(ev)); return (odp_packet_t)ev; }
All examples of odp code do not check odp_schedule() output, I.e.: ev = odp_schedule() pkt = odp_packet_from_event(ev) Because of ev can be not only packet (timer, crypto operation), add check that only packet event can be converted to packet. Signed-off-by: Maxim Uvarov <maxim.uvarov@linaro.org> --- After Zoltan's changes to API-NEXT I think it's reasonable to add check to odp_packet_from_event() to make linux-generic catch other non packets events. Thanks, Maxim. platform/linux-generic/odp_crypto.c | 2 +- platform/linux-generic/odp_packet.c | 3 +++ 2 files changed, 4 insertions(+), 1 deletion(-)