muvarov replied on github web page:
platform/linux-generic/odp_packet.c
line 23
@@ -1739,9 +1739,68 @@ void odp_packet_print(odp_packet_t pkt)
seg = odp_packet_next_seg(pkt, seg);
}
- str[len] = '\0';
+ ODP_PRINT("%s\n", str);
+}
+
+void odp_packet_print_data(odp_packet_t pkt, uint32_t offset,
+ uint32_t byte_len)
+{
+ odp_packet_hdr_t *hdr = packet_hdr(pkt);
+ uint32_t bytes_per_row = 16;
+ int num_rows = (byte_len + bytes_per_row - 1) / bytes_per_row;
+ int max_len = 256 + (3 * byte_len) + (3 * num_rows);
+ char str[max_len];
+ int len = 0;
+ int n = max_len - 1;
+ uint32_t data_len = odp_packet_len(pkt);
+ pool_t *pool = hdr->buf_hdr.pool_ptr;
+
+ len += snprintf(&str[len], n - len, "Packet\n------\n");
+ len += snprintf(&str[len], n - len,
+ " pool index %" PRIu32 "\n", pool->pool_idx);
Comment:
@lumag like here:
https://answers.launchpad.net/gcc-arm-embedded/+question/293823 old gcc looks
like failed. But int usually = uint32_t, so it can work on both cases. I
prefer see exact type.
> Petri Savolainen(psavol) wrote:
> I understand the concern, but I don't want to invent a new snprintf pattern
> in this patch set (since the purpose is API change). If / when a new pattern
> is introduced, it should be then used all over, otherwise we'll have multiple
> patterns ...
>> Ilias Apalodimas(apalos) wrote:
>> My only concern is that it is relatively easy to miss that on a future
>> review(a commit that just adds snprintf lines without increasing the size),
>> but we mostly agree that's why i added this only as a comment.
>> I think we'll need a wrapper over snprintf to be safe in the future.
>>> Petri Savolainen(psavol) wrote:
>>> This patter is used in many places already. Including the
>>> odp_packet_print() function which is just above this function in the same
>>> file. I think "someone" modifying a line also need to know what he's doing
>>> and update pre-calculated string length. Someone may insert a bug in many
>>> ways... with or without a check on these lines.
>>>
>>> Anyway, the point in this patch series is to add a new API function, and
>>> provide a simple implementation and test for that. Once the new API is
>>> merged, the implementation maybe modified / improved many times. If this
>>> function needs those checks, then should all other similar functions in our
>>> code base. That code base clean up could be done as a separate patch set
>>> (once the correct snprintf pattern has been agreed in the community).
>>>> Ilias Apalodimas(apalos) wrote:
>>>> This is a standard pattern for buffer overflows. This code assumes
>>>> snprintf returns the bytes it wrote which is not always correct. If
>>>> someone adds extra snprintf statements without increasing max_len this
>>>> would lead to overflows.
>>>>> Petri Savolainen(psavol) wrote:
>>>>> Yes there is. This avoids interleaved output when two (or more) threads
>>>>> print simultaneously. Output is pretty useless if lines are interleaved.
>>>>> Also, the current packet and buffer print functions do the same.
>>>>>> Dmitry Eremin-Solenikov(lumag) wrote:
>>>>>> @muvarov they do not mention any compiler errors, so they are useless in
>>>>>> trying to understand, which errors were there.
>>>>>>> Bill Fischofer(Bill-Fischofer-Linaro) wrote:
>>>>>>> It's never wrong to use the PRIxxx macros so that's the safest and most
>>>>>>> portable way to go. So I'm happy to see them used here and we should
>>>>>>> try to use them in new development that calls for these sort of
>>>>>>> conversions.
>>>>>>>> muvarov wrote
>>>>>>>> @lumag compilation errors. So we have comits:
>>>>>>>> 27a05a1d61f3b7e4d5
>>>>>>>>
>>>>>>>> 42d0b99c453f269f
>>>>>>>>
>>>>>>>> d25d276c649f57021f
>>>>>>>>> Dmitry Eremin-Solenikov(lumag) wrote:
>>>>>>>>> @muvarov could you please be more specific? What kind of problems?
>>>>>>>>>> muvarov wrote
>>>>>>>>>> we had some problems with that prints in the past. Arm build or -m32
>>>>>>>>>> do not remember exactly what it was.
>>>>>>>>>>> Dmitry Eremin-Solenikov(lumag) wrote:
>>>>>>>>>>> My understanding is that `PRIu32/PRIu16` are not required, you can
>>>>>>>>>>> always use `%u/%hu` instead. Only `PRIu64/PRIx64` are required,
>>>>>>>>>>> because exact `uint64_t` definition depends on platform ABI.
>>>>>>>>>>>> Dmitry Eremin-Solenikov(lumag) wrote:
>>>>>>>>>>>> Is there any reason for printing into the string buffer, rather
>>>>>>>>>>>> than calling `ODP_PRINT` directly?
https://github.com/Linaro/odp/pull/258#discussion_r147438331
updated_at 2017-10-27 15:14:41
