Re: [lng-odp] [API-NEXT PATCH 1/7] api: packet: src and dst packet must not be the same

[Maxim Uvarov]

Merged,
Maxim.

On 12/23/16 11:14, Bala Manoharan wrote:
> Reviewed-by: Balasubramanian Manoharan 
> 
> On 22 December 2016 at 20:03, Petri Savolainen
>  wrote:
>> Concat and copy_from_pkt operations must be called with src and
>> dst packet handles which refer to the same packet.
>>
>> Signed-off-by: Petri Savolainen 
>> ---
>>  include/odp/api/spec/packet.h | 6 +-
>>  1 file changed, 5 insertions(+), 1 deletion(-)
>>
>> diff --git a/include/odp/api/spec/packet.h b/include/odp/api/spec/packet.h
>> index faf62e2..4a86eba 100644
>> --- a/include/odp/api/spec/packet.h
>> +++ b/include/odp/api/spec/packet.h
>> @@ -781,7 +781,8 @@ uint32_t odp_packet_seg_data_len(odp_packet_t pkt, 
>> odp_packet_seg_t seg);
>>   * Concatenate all packet data from 'src' packet into tail of 'dst' packet.
>>   * Operation preserves 'dst' packet metadata in the resulting packet,
>>   * while 'src' packet handle, metadata and old segment handles for both 
>> packets
>> - * become invalid.
>> + * become invalid. Source and destination packet handles must not refer to
>> + * the same packet.
>>   *
>>   * A successful operation overwrites 'dst' packet handle with a new handle,
>>   * which application must use as the reference to the resulting packet
>> @@ -928,6 +929,9 @@ int odp_packet_copy_from_mem(odp_packet_t pkt, uint32_t 
>> offset,
>>   * Copy 'len' bytes of data from 'src' packet to 'dst' packet. Copy starts 
>> from
>>   * the specified source and destination packet offsets. Copied areas
>>   * (offset ... offset + len) must not exceed their packet data lengths.
>> + * Source and destination packet handles must not refer to the same packet 
>> (use
>> + * odp_packet_copy_data() or odp_packet_move_data() for a single packet).
>> + *
>>   * Packet is not modified on an error.
>>   *
>>   * @param dstDestination packet handle
>> --
>> 2.8.1
>>

Re: [lng-odp] [API-NEXT PATCH 1/7] api: packet: src and dst packet must not be the same

[Bill Fischofer]

OK.

For this series:

Reviewed-and-tested-by: Bill Fischofer <bill.fischo...@linaro.org>

On Fri, Dec 23, 2016 at 1:56 AM, Savolainen, Petri (Nokia - FI/Espoo)
<petri.savolai...@nokia-bell-labs.com> wrote:
>
>
>> -Original Message-
>> From: Bill Fischofer [mailto:bill.fischo...@linaro.org]
>> Sent: Thursday, December 22, 2016 6:59 PM
>> To: Savolainen, Petri (Nokia - FI/Espoo) <petri.savolainen@nokia-bell-
>> labs.com>
>> Cc: lng-odp-forward <lng-odp@lists.linaro.org>
>> Subject: Re: [lng-odp] [API-NEXT PATCH 1/7] api: packet: src and dst
>> packet must not be the same
>>
>> On Thu, Dec 22, 2016 at 8:33 AM, Petri Savolainen
>> <petri.savolai...@nokia.com> wrote:
>> > Concat and copy_from_pkt operations must be called with src and
>> > dst packet handles which refer to the same packet.
>> >
>> > Signed-off-by: Petri Savolainen <petri.savolai...@nokia.com>
>> > ---
>> >  include/odp/api/spec/packet.h | 6 +-
>> >  1 file changed, 5 insertions(+), 1 deletion(-)
>> >
>> > diff --git a/include/odp/api/spec/packet.h
>> b/include/odp/api/spec/packet.h
>> > index faf62e2..4a86eba 100644
>> > --- a/include/odp/api/spec/packet.h
>> > +++ b/include/odp/api/spec/packet.h
>> > @@ -781,7 +781,8 @@ uint32_t odp_packet_seg_data_len(odp_packet_t pkt,
>> odp_packet_seg_t seg);
>> >   * Concatenate all packet data from 'src' packet into tail of 'dst'
>> packet.
>> >   * Operation preserves 'dst' packet metadata in the resulting packet,
>> >   * while 'src' packet handle, metadata and old segment handles for both
>> packets
>> > - * become invalid.
>> > + * become invalid. Source and destination packet handles must not refer
>> to
>> > + * the same packet.
>>
>> Why introduce this restriction? Concatenating a packet to itself is a
>> well-defined operation and is supported by other languages that
>> provide similar concatenation operations.
>
> Concat is not the same as copy. Application forms a new packet from two old 
> packets. If application wants to duplicate data in one packet it must do a 
> copy. If you concat a packet into itself, you form a loop which does not make 
> any sense.
>
>
>>
>> >   *
>> >   * A successful operation overwrites 'dst' packet handle with a new
>> handle,
>> >   * which application must use as the reference to the resulting packet
>> > @@ -928,6 +929,9 @@ int odp_packet_copy_from_mem(odp_packet_t pkt,
>> uint32_t offset,
>> >   * Copy 'len' bytes of data from 'src' packet to 'dst' packet. Copy
>> starts from
>> >   * the specified source and destination packet offsets. Copied areas
>> >   * (offset ... offset + len) must not exceed their packet data lengths.
>> > + * Source and destination packet handles must not refer to the same
>> packet (use
>> > + * odp_packet_copy_data() or odp_packet_move_data() for a single
>> packet).
>>
>> Same question here. It's simple enough for the implementation to check
>> this and behave appropriately (as the current implementation does).
>> Why introduce this change now?
>
>
> As the documentation change highlights: we have odp_packet_copy_data() and 
> odp_packet_move_data() defined exactly for the case of copying or moving data 
> within one packet. odp_packet_copy_from_pkt() is defined for copying data 
> between two different packets. Application must use the right tool for the 
> problem, instead of implementation needing to check every time if application 
> happens to use a wrong tool.
>
> So, it's not change in behavior, but more explicit definition of the purpose 
> of the existing calls.
>
> -Petri
>
>

Re: [lng-odp] [API-NEXT PATCH 1/7] api: packet: src and dst packet must not be the same

[Bala Manoharan]

Reviewed-by: Balasubramanian Manoharan 

On 22 December 2016 at 20:03, Petri Savolainen
 wrote:
> Concat and copy_from_pkt operations must be called with src and
> dst packet handles which refer to the same packet.
>
> Signed-off-by: Petri Savolainen 
> ---
>  include/odp/api/spec/packet.h | 6 +-
>  1 file changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/include/odp/api/spec/packet.h b/include/odp/api/spec/packet.h
> index faf62e2..4a86eba 100644
> --- a/include/odp/api/spec/packet.h
> +++ b/include/odp/api/spec/packet.h
> @@ -781,7 +781,8 @@ uint32_t odp_packet_seg_data_len(odp_packet_t pkt, 
> odp_packet_seg_t seg);
>   * Concatenate all packet data from 'src' packet into tail of 'dst' packet.
>   * Operation preserves 'dst' packet metadata in the resulting packet,
>   * while 'src' packet handle, metadata and old segment handles for both 
> packets
> - * become invalid.
> + * become invalid. Source and destination packet handles must not refer to
> + * the same packet.
>   *
>   * A successful operation overwrites 'dst' packet handle with a new handle,
>   * which application must use as the reference to the resulting packet
> @@ -928,6 +929,9 @@ int odp_packet_copy_from_mem(odp_packet_t pkt, uint32_t 
> offset,
>   * Copy 'len' bytes of data from 'src' packet to 'dst' packet. Copy starts 
> from
>   * the specified source and destination packet offsets. Copied areas
>   * (offset ... offset + len) must not exceed their packet data lengths.
> + * Source and destination packet handles must not refer to the same packet 
> (use
> + * odp_packet_copy_data() or odp_packet_move_data() for a single packet).
> + *
>   * Packet is not modified on an error.
>   *
>   * @param dstDestination packet handle
> --
> 2.8.1
>

Re: [lng-odp] [API-NEXT PATCH 1/7] api: packet: src and dst packet must not be the same

[Savolainen, Petri (Nokia - FI/Espoo)]

> -Original Message-
> From: Bill Fischofer [mailto:bill.fischo...@linaro.org]
> Sent: Thursday, December 22, 2016 6:59 PM
> To: Savolainen, Petri (Nokia - FI/Espoo) <petri.savolainen@nokia-bell-
> labs.com>
> Cc: lng-odp-forward <lng-odp@lists.linaro.org>
> Subject: Re: [lng-odp] [API-NEXT PATCH 1/7] api: packet: src and dst
> packet must not be the same
> 
> On Thu, Dec 22, 2016 at 8:33 AM, Petri Savolainen
> <petri.savolai...@nokia.com> wrote:
> > Concat and copy_from_pkt operations must be called with src and
> > dst packet handles which refer to the same packet.
> >
> > Signed-off-by: Petri Savolainen <petri.savolai...@nokia.com>
> > ---
> >  include/odp/api/spec/packet.h | 6 +-
> >  1 file changed, 5 insertions(+), 1 deletion(-)
> >
> > diff --git a/include/odp/api/spec/packet.h
> b/include/odp/api/spec/packet.h
> > index faf62e2..4a86eba 100644
> > --- a/include/odp/api/spec/packet.h
> > +++ b/include/odp/api/spec/packet.h
> > @@ -781,7 +781,8 @@ uint32_t odp_packet_seg_data_len(odp_packet_t pkt,
> odp_packet_seg_t seg);
> >   * Concatenate all packet data from 'src' packet into tail of 'dst'
> packet.
> >   * Operation preserves 'dst' packet metadata in the resulting packet,
> >   * while 'src' packet handle, metadata and old segment handles for both
> packets
> > - * become invalid.
> > + * become invalid. Source and destination packet handles must not refer
> to
> > + * the same packet.
> 
> Why introduce this restriction? Concatenating a packet to itself is a
> well-defined operation and is supported by other languages that
> provide similar concatenation operations.

Concat is not the same as copy. Application forms a new packet from two old 
packets. If application wants to duplicate data in one packet it must do a 
copy. If you concat a packet into itself, you form a loop which does not make 
any sense. 


> 
> >   *
> >   * A successful operation overwrites 'dst' packet handle with a new
> handle,
> >   * which application must use as the reference to the resulting packet
> > @@ -928,6 +929,9 @@ int odp_packet_copy_from_mem(odp_packet_t pkt,
> uint32_t offset,
> >   * Copy 'len' bytes of data from 'src' packet to 'dst' packet. Copy
> starts from
> >   * the specified source and destination packet offsets. Copied areas
> >   * (offset ... offset + len) must not exceed their packet data lengths.
> > + * Source and destination packet handles must not refer to the same
> packet (use
> > + * odp_packet_copy_data() or odp_packet_move_data() for a single
> packet).
> 
> Same question here. It's simple enough for the implementation to check
> this and behave appropriately (as the current implementation does).
> Why introduce this change now?


As the documentation change highlights: we have odp_packet_copy_data() and 
odp_packet_move_data() defined exactly for the case of copying or moving data 
within one packet. odp_packet_copy_from_pkt() is defined for copying data 
between two different packets. Application must use the right tool for the 
problem, instead of implementation needing to check every time if application 
happens to use a wrong tool.

So, it's not change in behavior, but more explicit definition of the purpose of 
the existing calls.

-Petri

Re: [lng-odp] [API-NEXT PATCH 1/7] api: packet: src and dst packet must not be the same

[Bill Fischofer]

On Thu, Dec 22, 2016 at 8:33 AM, Petri Savolainen
 wrote:
> Concat and copy_from_pkt operations must be called with src and
> dst packet handles which refer to the same packet.
>
> Signed-off-by: Petri Savolainen 
> ---
>  include/odp/api/spec/packet.h | 6 +-
>  1 file changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/include/odp/api/spec/packet.h b/include/odp/api/spec/packet.h
> index faf62e2..4a86eba 100644
> --- a/include/odp/api/spec/packet.h
> +++ b/include/odp/api/spec/packet.h
> @@ -781,7 +781,8 @@ uint32_t odp_packet_seg_data_len(odp_packet_t pkt, 
> odp_packet_seg_t seg);
>   * Concatenate all packet data from 'src' packet into tail of 'dst' packet.
>   * Operation preserves 'dst' packet metadata in the resulting packet,
>   * while 'src' packet handle, metadata and old segment handles for both 
> packets
> - * become invalid.
> + * become invalid. Source and destination packet handles must not refer to
> + * the same packet.

Why introduce this restriction? Concatenating a packet to itself is a
well-defined operation and is supported by other languages that
provide similar concatenation operations.

>   *
>   * A successful operation overwrites 'dst' packet handle with a new handle,
>   * which application must use as the reference to the resulting packet
> @@ -928,6 +929,9 @@ int odp_packet_copy_from_mem(odp_packet_t pkt, uint32_t 
> offset,
>   * Copy 'len' bytes of data from 'src' packet to 'dst' packet. Copy starts 
> from
>   * the specified source and destination packet offsets. Copied areas
>   * (offset ... offset + len) must not exceed their packet data lengths.
> + * Source and destination packet handles must not refer to the same packet 
> (use
> + * odp_packet_copy_data() or odp_packet_move_data() for a single packet).

Same question here. It's simple enough for the implementation to check
this and behave appropriately (as the current implementation does).
Why introduce this change now?

> + *
>   * Packet is not modified on an error.
>   *
>   * @param dstDestination packet handle
> --
> 2.8.1
>