[API-NEXT] linux-generic: check for event type while converting packet to event

Message ID 1436183828-7364-1-git-send-email-maxim.uvarov@linaro.org
State New
Headers show

Commit Message

Maxim Uvarov July 6, 2015, 11:57 a.m.
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(-)

Comments

Bill Fischofer July 6, 2015, 11:59 a.m. | #1
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
>
Maxim Uvarov July 6, 2015, 12:04 p.m. | #2
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
>
>
Bill Fischofer July 6, 2015, 12:08 p.m. | #3
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
>>
>>
>>
>
Maxim Uvarov July 6, 2015, 12:11 p.m. | #4
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
>
>
>
>

Patch

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;
 }