diff mbox

Init ip header's frag_offset to 0.

Message ID 1412932948-5914-1-git-send-email-weilong.chen@linaro.org
State New
Headers show

Commit Message

Weilong Chen Oct. 10, 2014, 9:22 a.m. UTC
The receive check frag_offset to find if the packet's frags.
We should make sure this field is 0.

Signed-off-by: Weilong Chen <weilong.chen@linaro.org>
---
 example/generator/odp_generator.c |    2 ++
 1 file changed, 2 insertions(+)

Comments

Ciprian Barbu Oct. 14, 2014, 8:44 a.m. UTC | #1
On Fri, Oct 10, 2014 at 12:22 PM, Weilong Chen <weilong.chen@linaro.org> wrote:
> The receive check frag_offset to find if the packet's frags.
> We should make sure this field is 0.
>
> Signed-off-by: Weilong Chen <weilong.chen@linaro.org>
> ---
>  example/generator/odp_generator.c |    2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/example/generator/odp_generator.c b/example/generator/odp_generator.c
> index 6055324..421fe8e 100644
> --- a/example/generator/odp_generator.c
> +++ b/example/generator/odp_generator.c
> @@ -203,6 +203,7 @@ static void pack_udp_pkt(odp_buffer_t obuf)
>         ip->proto = ODPH_IPPROTO_UDP;
>         seq = odp_atomic_fetch_add_u64(&counters.seq, 1) % 0xFFFF;
>         ip->id = odp_cpu_to_be_16(seq);
> +       ip->frag_offset = 0;

You still have ttl and tos uninitialized from the ip structure. I
don't know about tos, but ttl should be set to something I think.

Also one more thing about packet creation which I noticed a while
back. Odp_generator allocates and manipulates buffers instead of
packets. I think it's more appropriate to use packet APIs instead of
buffer APIs where possible since it is known that packets are needed.
I.e. replacing odp_buffer_alloc with odp_packet_alloc,
odp_buffer_valid with odp_packet_is_valid etc. Also in the
pack_udp_pkt function for example the data pointer should be retrieved
with other specialized API, like odp_packet_start or odp_packet_addr
when it is introduced from the approved Packet Design Document.

Would like some more opinions on this.

>         ip->chksum = 0;
>         odph_ipv4_csum_update(pkt);
>         /* udp */
> @@ -260,6 +261,7 @@ static void pack_icmp_pkt(odp_buffer_t obuf)
>         ip->proto = ODPH_IPPROTO_ICMP;
>         seq = odp_atomic_fetch_add_u64(&counters.seq, 1) % 0xffff;
>         ip->id = odp_cpu_to_be_16(seq);
> +       ip->frag_offset = 0;
>         ip->chksum = 0;
>         odph_ipv4_csum_update(pkt);
>         /* icmp */
> --
> 1.7.9.5
>
>
> _______________________________________________
> lng-odp mailing list
> lng-odp@lists.linaro.org
> http://lists.linaro.org/mailman/listinfo/lng-odp
Weilong Chen Oct. 15, 2014, 10:37 a.m. UTC | #2
Good,
Thanks

On 14 October 2014 16:44, Ciprian Barbu <ciprian.barbu@linaro.org> wrote:

> On Fri, Oct 10, 2014 at 12:22 PM, Weilong Chen <weilong.chen@linaro.org>
> wrote:
> > The receive check frag_offset to find if the packet's frags.
> > We should make sure this field is 0.
> >
> > Signed-off-by: Weilong Chen <weilong.chen@linaro.org>
> > ---
> >  example/generator/odp_generator.c |    2 ++
> >  1 file changed, 2 insertions(+)
> >
> > diff --git a/example/generator/odp_generator.c
> b/example/generator/odp_generator.c
> > index 6055324..421fe8e 100644
> > --- a/example/generator/odp_generator.c
> > +++ b/example/generator/odp_generator.c
> > @@ -203,6 +203,7 @@ static void pack_udp_pkt(odp_buffer_t obuf)
> >         ip->proto = ODPH_IPPROTO_UDP;
> >         seq = odp_atomic_fetch_add_u64(&counters.seq, 1) % 0xFFFF;
> >         ip->id = odp_cpu_to_be_16(seq);
> > +       ip->frag_offset = 0;
>
> You still have ttl and tos uninitialized from the ip structure. I
> don't know about tos, but ttl should be set to something I think.
>
> Also one more thing about packet creation which I noticed a while
> back. Odp_generator allocates and manipulates buffers instead of
> packets. I think it's more appropriate to use packet APIs instead of
> buffer APIs where possible since it is known that packets are needed.
> I.e. replacing odp_buffer_alloc with odp_packet_alloc,
> odp_buffer_valid with odp_packet_is_valid etc. Also in the
> pack_udp_pkt function for example the data pointer should be retrieved
> with other specialized API, like odp_packet_start or odp_packet_addr
> when it is introduced from the approved Packet Design Document.
>
> Would like some more opinions on this.
>
> >         ip->chksum = 0;
> >         odph_ipv4_csum_update(pkt);
> >         /* udp */
> > @@ -260,6 +261,7 @@ static void pack_icmp_pkt(odp_buffer_t obuf)
> >         ip->proto = ODPH_IPPROTO_ICMP;
> >         seq = odp_atomic_fetch_add_u64(&counters.seq, 1) % 0xffff;
> >         ip->id = odp_cpu_to_be_16(seq);
> > +       ip->frag_offset = 0;
> >         ip->chksum = 0;
> >         odph_ipv4_csum_update(pkt);
> >         /* icmp */
> > --
> > 1.7.9.5
> >
> >
> > _______________________________________________
> > lng-odp mailing list
> > lng-odp@lists.linaro.org
> > http://lists.linaro.org/mailman/listinfo/lng-odp
>
diff mbox

Patch

diff --git a/example/generator/odp_generator.c b/example/generator/odp_generator.c
index 6055324..421fe8e 100644
--- a/example/generator/odp_generator.c
+++ b/example/generator/odp_generator.c
@@ -203,6 +203,7 @@  static void pack_udp_pkt(odp_buffer_t obuf)
 	ip->proto = ODPH_IPPROTO_UDP;
 	seq = odp_atomic_fetch_add_u64(&counters.seq, 1) % 0xFFFF;
 	ip->id = odp_cpu_to_be_16(seq);
+	ip->frag_offset = 0;
 	ip->chksum = 0;
 	odph_ipv4_csum_update(pkt);
 	/* udp */
@@ -260,6 +261,7 @@  static void pack_icmp_pkt(odp_buffer_t obuf)
 	ip->proto = ODPH_IPPROTO_ICMP;
 	seq = odp_atomic_fetch_add_u64(&counters.seq, 1) % 0xffff;
 	ip->id = odp_cpu_to_be_16(seq);
+	ip->frag_offset = 0;
 	ip->chksum = 0;
 	odph_ipv4_csum_update(pkt);
 	/* icmp */