On 03/27/15 19:21, Mike Holmes wrote:
ODP agreed that only critical bugs hold the release at the last
minuet, this issue is not even a bug at this point - it should be so
that users are aware of the issue.
If this really is affecting a critical product a bug should be created
with the rational for this to be in 1.0.2 and we can break our rules
and work overtime testing it again - last minuet changes require effort.
In this case only local developer tests might be affected - no one has
noticed this yet, the fix is available on the list, it is likely to be
in master on Monday and will be in 1.0.3 in two weeks, given that I
cant see the urgency yet.
I agree we want the fix in, but this has not even been on the list 24
hours which is a second gating item for ODP.
+1 no rush with such changes. That will be in 1.0.3
Maxim.
On 27 March 2015 at 10:33, Bill Fischofer <[email protected]
<mailto:[email protected]>> wrote:
Silent memory corruption is a problem even if specific tests don't
reveal it. Due to the pool memory layout, the corruption is
writing something into some other buffer, which may or may not be
allocated. That's always a recipe for sudden and confusing
failures so I'd view this as a critical bug.
On Fri, Mar 27, 2015 at 9:11 AM, Stuart Haslam
<[email protected] <mailto:[email protected]>> wrote:
It doesn't break non-jumbo tests, in fact all tests pass and I
didn't *notice* any adverse effects, I found the issue by
inspection.
The problem is that when generating non-jumbo packets (which
most of the tests do) the magic2 marker is written beyond the
end of a calloc'd block into un-allocated memory. The receiver
doesn't check magic2 for non-jumbo packets.
The issue could be trivially avoided with the simpler change
below, but I wasn't happy with that as we'd still have a
pointer to a structure that's much larger than the memory
allocated.
Stuart.
diff --git a/test/validation/odp_pktio.c
b/test/validation/odp_pktio.c
index e022c33..5ea45e4 100644
--- a/test/validation/odp_pktio.c
+++ b/test/validation/odp_pktio.c
@@ -95,7 +95,8 @@ static int pktio_pkt_set_seq(odp_packet_t pkt)
CU_ASSERT_FATAL(data != NULL);
data->head.magic = TEST_SEQ_MAGIC;
- data->magic2 = TEST_SEQ_MAGIC;
+ if (test_jumbo)
+ data->magic2 = TEST_SEQ_MAGIC;
data->head.seq = tstseq;
odp_packet_copydata_in(pkt, l4_off+ODPH_UDPHDR_LEN,
On 27 March 2015 at 13:15, Bill Fischofer
<[email protected] <mailto:[email protected]>>
wrote:
If I understand Stuart's patch correctly, he's saying that
the introduction of jumbo frame support in v1.0.1 broke
non-jumbo processing. Since that's a critical function I'd
think we'd want to fix that bug ASAP in v1.0.2.
On Fri, Mar 27, 2015 at 7:04 AM, Mike Holmes
<[email protected] <mailto:[email protected]>>
wrote:
On 26 March 2015 at 17:14, Bill Fischofer
<[email protected]
<mailto:[email protected]>> wrote:
Sounds like we should hold v1.0.2 for this bug
fix? Maxim: I think this review is properly yours
as you did the jumbo frame add for v1.0.1.
Bill can you describe the use case than for holding,
if we have a strong case for it then we can hold.
Normal procedure is that whatever makes the cut is
there and the release goes out unless there is a
regression introduced.
On Thu, Mar 26, 2015 at 12:52 PM, Stuart Haslam
<[email protected]
<mailto:[email protected]>> wrote:
A magic number, used to ensure jumbo frames
are transmitted and received
in full, is written to the end of a packet
buffer at a fixed offset of
9170 bytes. However for non-jumbo tests this
is way beyond the end of
the allocated buffer.
Signed-off-by: Stuart Haslam
<[email protected]
<mailto:[email protected]>>
---
test/validation/odp_pktio.c | 84
++++++++++++++++++++++++++++-----------------
1 file changed, 52 insertions(+), 32 deletions(-)
diff --git a/test/validation/odp_pktio.c
b/test/validation/odp_pktio.c
index e022c33..d653da4 100644
--- a/test/validation/odp_pktio.c
+++ b/test/validation/odp_pktio.c
@@ -44,7 +44,6 @@ typedef struct ODP_PACKED {
/** structure of test packet UDP payload */
typedef struct ODP_PACKED {
- pkt_head_t head;
char data[PKT_BUF_JUMBO_MAX_PAYLOAD -
sizeof(pkt_head_t) -
sizeof(uint32be_t)];
uint32be_t magic2;
@@ -74,33 +73,47 @@ static void
pktio_pkt_set_macs(odp_packet_t pkt,
static uint32_t pkt_payload_len(void)
{
- return test_jumbo ?
sizeof(pkt_test_data_t) : sizeof(pkt_head_t);
+ uint32_t len;
+
+ len = sizeof(pkt_head_t);
+
+ if (test_jumbo)
+ len += sizeof(pkt_test_data_t);
+
+ return len;
}
static int pktio_pkt_set_seq(odp_packet_t pkt)
{
static uint32_t tstseq;
- size_t l4_off;
- pkt_test_data_t *data;
- uint32_t len = pkt_payload_len();
-
+ size_t off;
+ pkt_head_t head;
- l4_off = odp_packet_l4_offset(pkt);
- if (!l4_off) {
+ off = odp_packet_l4_offset(pkt);
+ if (!off) {
CU_FAIL("packet L4 offset not set");
return -1;
}
- data = calloc(1, len);
- CU_ASSERT_FATAL(data != NULL);
+ head.magic = TEST_SEQ_MAGIC;
+ head.seq = tstseq;
+
+ off += ODPH_UDPHDR_LEN;
+ odp_packet_copydata_in(pkt, off,
sizeof(head), &head);
+
+ if (test_jumbo) {
+ pkt_test_data_t *data;
+
+ data = calloc(1, sizeof(*data));
+ CU_ASSERT_FATAL(data != NULL);
- data->head.magic = TEST_SEQ_MAGIC;
- data->magic2 = TEST_SEQ_MAGIC;
- data->head.seq = tstseq;
+ data->magic2 = TEST_SEQ_MAGIC;
+
+ off += sizeof(head);
+ odp_packet_copydata_in(pkt, off,
sizeof(*data), data);
+ free(data);
+ }
- odp_packet_copydata_in(pkt,
l4_off+ODPH_UDPHDR_LEN,
- len, data);
- free(data);
tstseq++;
return 0;
@@ -108,30 +121,37 @@ static int
pktio_pkt_set_seq(odp_packet_t pkt)
static uint32_t pktio_pkt_seq(odp_packet_t pkt)
{
- size_t l4_off;
+ size_t off;
uint32_t seq = TEST_SEQ_INVALID;
- pkt_test_data_t *data;
- uint32_t len = pkt_payload_len();
+ pkt_head_t head;
- l4_off = odp_packet_l4_offset(pkt);
- if (l4_off == ODP_PACKET_OFFSET_INVALID)
+ off = odp_packet_l4_offset(pkt);
+ if (off == ODP_PACKET_OFFSET_INVALID)
return TEST_SEQ_INVALID;
- data = calloc(1, len);
- CU_ASSERT_FATAL(data != NULL);
+ off += ODPH_UDPHDR_LEN;
+ odp_packet_copydata_out(pkt, off,
sizeof(head), &head);
- odp_packet_copydata_out(pkt,
l4_off+ODPH_UDPHDR_LEN,
- len, data);
+ if (head.magic != TEST_SEQ_MAGIC)
+ return TEST_SEQ_INVALID;
- if (data->head.magic == TEST_SEQ_MAGIC) {
- if (test_jumbo && data->magic2 !=
TEST_SEQ_MAGIC) {
- free(data);
- return TEST_SEQ_INVALID;
- }
- seq = data->head.seq;
+ seq = head.seq;
+
+ if (test_jumbo) {
+ pkt_test_data_t *data;
+
+ data = calloc(1, sizeof(*data));
+ CU_ASSERT_FATAL(data != NULL);
+
+ off += sizeof(head);
+ odp_packet_copydata_out(pkt, off,
sizeof(*data), data);
+
+ if (data->magic2 != TEST_SEQ_MAGIC)
+ seq = TEST_SEQ_INVALID;
+
+ free(data);
}
- free(data);
return seq;
}
--
2.1.1
_______________________________________________
lng-odp mailing list
[email protected]
<mailto:[email protected]>
http://lists.linaro.org/mailman/listinfo/lng-odp
_______________________________________________
lng-odp mailing list
[email protected]
<mailto:[email protected]>
http://lists.linaro.org/mailman/listinfo/lng-odp
--
Mike Holmes
Technical Manager - Linaro Networking Group
Linaro.org <http://www.linaro.org/>***│ *Open source
software for ARM SoCs
--
Mike Holmes
Technical Manager - Linaro Networking Group
Linaro.org <http://www.linaro.org/>***│ *Open source software for ARM SoCs
_______________________________________________
lng-odp mailing list
[email protected]
http://lists.linaro.org/mailman/listinfo/lng-odp
_______________________________________________
lng-odp mailing list
[email protected]
http://lists.linaro.org/mailman/listinfo/lng-odp
