| Message ID | 1401729001-15729-3-git-send-email-stuart.haslam@arm.com |
|---|---|
| State | Accepted |
| Commit | ac3af625e59510ed86e7320487fca6948d2c8e23 |
| Headers | show |
On 2014-06-02 18:10, Stuart Haslam wrote: > ODP_PACKET_OFFSET_INVALID is defined as ((size_t)-1) but the existing > implementations both store offsets using a uint32_t, which breaks things > in a few places on 64-bit systems. There's some explicit casting in the > implementations but it's not consistent and the odp_packet_l2/3/4 > functions can return a junk ptr if packet parsing didn't get far enough > to set the offset correctly. > > Signed-off-by: Stuart Haslam <stuart.haslam@arm.com> > --- > include/odp_packet.h | 2 +- > platform/linux-generic/source/odp_packet.c | 6 +++--- > platform/linux-keystone2/source/odp_packet.c | 6 +++--- > 3 files changed, 7 insertions(+), 7 deletions(-) > > diff --git a/include/odp_packet.h b/include/odp_packet.h > index f7014fb..932b009 100644 > --- a/include/odp_packet.h > +++ b/include/odp_packet.h > @@ -30,7 +30,7 @@ typedef uint32_t odp_packet_t; > #define ODP_PACKET_INVALID ODP_BUFFER_INVALID > > /** Invalid offset */ > -#define ODP_PACKET_OFFSET_INVALID ((size_t)-1) > +#define ODP_PACKET_OFFSET_INVALID ((uint32_t)-1) Why can't this be the size_t? > > > /** > diff --git a/platform/linux-generic/source/odp_packet.c b/platform/linux-generic/source/odp_packet.c > index eb7c227..530e513 100644 > --- a/platform/linux-generic/source/odp_packet.c > +++ b/platform/linux-generic/source/odp_packet.c > @@ -31,9 +31,9 @@ void odp_packet_init(odp_packet_t pkt) > len = ODP_OFFSETOF(odp_packet_hdr_t, payload) - start_offset; > memset(start, 0, len); > > - pkt_hdr->l2_offset = (uint32_t) ODP_PACKET_OFFSET_INVALID; > - pkt_hdr->l3_offset = (uint32_t) ODP_PACKET_OFFSET_INVALID; > - pkt_hdr->l4_offset = (uint32_t) ODP_PACKET_OFFSET_INVALID; > + pkt_hdr->l2_offset = ODP_PACKET_OFFSET_INVALID; > + pkt_hdr->l3_offset = ODP_PACKET_OFFSET_INVALID; > + pkt_hdr->l4_offset = ODP_PACKET_OFFSET_INVALID; and change the structs odp_packet_hdr_t members to l2_offset, l3_offset and l4_offset to size_t? Cheers, Anders > } > > odp_packet_t odp_packet_from_buffer(odp_buffer_t buf) > diff --git a/platform/linux-keystone2/source/odp_packet.c b/platform/linux-keystone2/source/odp_packet.c > index 37a0d46..271d66b 100644 > --- a/platform/linux-keystone2/source/odp_packet.c > +++ b/platform/linux-keystone2/source/odp_packet.c > @@ -25,9 +25,9 @@ void odp_packet_init(odp_packet_t pkt) > { > odp_packet_hdr_t *const pkt_hdr = odp_packet_hdr(pkt); > > - pkt_hdr->l2_offset = (uint32_t) ODP_PACKET_OFFSET_INVALID; > - pkt_hdr->l3_offset = (uint32_t) ODP_PACKET_OFFSET_INVALID; > - pkt_hdr->l4_offset = (uint32_t) ODP_PACKET_OFFSET_INVALID; > + pkt_hdr->l2_offset = ODP_PACKET_OFFSET_INVALID; > + pkt_hdr->l3_offset = ODP_PACKET_OFFSET_INVALID; > + pkt_hdr->l4_offset = ODP_PACKET_OFFSET_INVALID; > } > > odp_packet_t odp_packet_from_buffer(odp_buffer_t buf) > -- > 1.9.1 > > > > _______________________________________________ > lng-odp mailing list > lng-odp@lists.linaro.org > http://lists.linaro.org/mailman/listinfo/lng-odp
On Wed, Jun 04, 2014 at 09:46:23PM +0100, Anders Roxell wrote: > On 2014-06-02 18:10, Stuart Haslam wrote: > > ODP_PACKET_OFFSET_INVALID is defined as ((size_t)-1) but the existing > > implementations both store offsets using a uint32_t, which breaks things > > in a few places on 64-bit systems. There's some explicit casting in the > > implementations but it's not consistent and the odp_packet_l2/3/4 > > functions can return a junk ptr if packet parsing didn't get far enough > > to set the offset correctly. > > > > Signed-off-by: Stuart Haslam <stuart.haslam@arm.com> > > --- > > include/odp_packet.h | 2 +- > > platform/linux-generic/source/odp_packet.c | 6 +++--- > > platform/linux-keystone2/source/odp_packet.c | 6 +++--- > > 3 files changed, 7 insertions(+), 7 deletions(-) > > > > diff --git a/include/odp_packet.h b/include/odp_packet.h > > index f7014fb..932b009 100644 > > --- a/include/odp_packet.h > > +++ b/include/odp_packet.h > > @@ -30,7 +30,7 @@ typedef uint32_t odp_packet_t; > > #define ODP_PACKET_INVALID ODP_BUFFER_INVALID > > > > /** Invalid offset */ > > -#define ODP_PACKET_OFFSET_INVALID ((size_t)-1) > > +#define ODP_PACKET_OFFSET_INVALID ((uint32_t)-1) > > Why can't this be the size_t? > It could be. I did it this way because the only existing implementations use a uint32_t and I wasn't keen on changing that to a size_t as 32-bits is already (more than) enough to represent packet offsets, and it would make the packet hdr 20 bytes bigger. I was also working on the assumption that if some future platform wanted a different value this #define would be moved into a platform specific header.
diff --git a/include/odp_packet.h b/include/odp_packet.h index f7014fb..932b009 100644 --- a/include/odp_packet.h +++ b/include/odp_packet.h @@ -30,7 +30,7 @@ typedef uint32_t odp_packet_t; #define ODP_PACKET_INVALID ODP_BUFFER_INVALID /** Invalid offset */ -#define ODP_PACKET_OFFSET_INVALID ((size_t)-1) +#define ODP_PACKET_OFFSET_INVALID ((uint32_t)-1) /** diff --git a/platform/linux-generic/source/odp_packet.c b/platform/linux-generic/source/odp_packet.c index eb7c227..530e513 100644 --- a/platform/linux-generic/source/odp_packet.c +++ b/platform/linux-generic/source/odp_packet.c @@ -31,9 +31,9 @@ void odp_packet_init(odp_packet_t pkt) len = ODP_OFFSETOF(odp_packet_hdr_t, payload) - start_offset; memset(start, 0, len); - pkt_hdr->l2_offset = (uint32_t) ODP_PACKET_OFFSET_INVALID; - pkt_hdr->l3_offset = (uint32_t) ODP_PACKET_OFFSET_INVALID; - pkt_hdr->l4_offset = (uint32_t) ODP_PACKET_OFFSET_INVALID; + pkt_hdr->l2_offset = ODP_PACKET_OFFSET_INVALID; + pkt_hdr->l3_offset = ODP_PACKET_OFFSET_INVALID; + pkt_hdr->l4_offset = ODP_PACKET_OFFSET_INVALID; } odp_packet_t odp_packet_from_buffer(odp_buffer_t buf) diff --git a/platform/linux-keystone2/source/odp_packet.c b/platform/linux-keystone2/source/odp_packet.c index 37a0d46..271d66b 100644 --- a/platform/linux-keystone2/source/odp_packet.c +++ b/platform/linux-keystone2/source/odp_packet.c @@ -25,9 +25,9 @@ void odp_packet_init(odp_packet_t pkt) { odp_packet_hdr_t *const pkt_hdr = odp_packet_hdr(pkt); - pkt_hdr->l2_offset = (uint32_t) ODP_PACKET_OFFSET_INVALID; - pkt_hdr->l3_offset = (uint32_t) ODP_PACKET_OFFSET_INVALID; - pkt_hdr->l4_offset = (uint32_t) ODP_PACKET_OFFSET_INVALID; + pkt_hdr->l2_offset = ODP_PACKET_OFFSET_INVALID; + pkt_hdr->l3_offset = ODP_PACKET_OFFSET_INVALID; + pkt_hdr->l4_offset = ODP_PACKET_OFFSET_INVALID; } odp_packet_t odp_packet_from_buffer(odp_buffer_t buf)
ODP_PACKET_OFFSET_INVALID is defined as ((size_t)-1) but the existing implementations both store offsets using a uint32_t, which breaks things in a few places on 64-bit systems. There's some explicit casting in the implementations but it's not consistent and the odp_packet_l2/3/4 functions can return a junk ptr if packet parsing didn't get far enough to set the offset correctly. Signed-off-by: Stuart Haslam <stuart.haslam@arm.com> --- include/odp_packet.h | 2 +- platform/linux-generic/source/odp_packet.c | 6 +++--- platform/linux-keystone2/source/odp_packet.c | 6 +++--- 3 files changed, 7 insertions(+), 7 deletions(-)