From: 蔡慧超 <chcch...@163.com>
Sent: Friday, October 8, 2021 10:38 AM
To: Ananyev, Konstantin <konstantin.anan...@intel.com>
Cc: cai...@chinatelecom.cn; dev@dpdk.org
Subject: Re:RE: Re:RE: [PATCH] ip_frag: modify the fragment offset and mf
Hi Konstantin,
Thank you for your reply, it's so useful！！！
But I have some questions.
Would you mind answer my questions？
>Cc: mailto:sta...@dpdk.org
This should be written in the commiter message：Cc:
sta...@dpdk.org，right<mailto:sta...@dpdk.org，right>?
[KA] Yes
When I send patch,what is the parameters of --subject-prefix?(Which branch(es)?)
--subject-prefix='PATCH vX’, where X is the number of current revision (2, 3, …)
I don't need to send patches to dev@dpdk.org<mailto:dev@dpdk.org> anymore，right?
No, you do.
I usually do something like that:
git send-email --to dev@dpdk.org<mailto:dev@dpdk.org> -cc <maintainer email>
--thread --no-chain-reply-to --in-reply-to="MSG ID of previous version of the
patch”
>Bugzilla ID
Let me start with Bugzilla-related material,
which will take some time and may be created later when other bugs are found.
Best regards.
Kevin
At 2021-10-08 16:33:49, "Ananyev, Konstantin"
<konstantin.anan...@intel.com<mailto:konstantin.anan...@intel.com>> wrote:
>Hi Kevin,
>
>I thought about something like that
>(feel free to update it if you feel I missed something):
>
>ip_frag: fix fragmenting IPv4 fragment
>
>Current implementation of rte_ipv4_fragment_packet() doesn’t take
>into account offset and flag values of the given packet, but blindly assumes
>they are always zero (original packet is not fragmented).
>According to RFC791, fragment and flag values for new fragment should
>take into account values provided in the original IPv4 packet.
>
>Fixes: 4c38e5532a07 ("ip_frag: refactor IPv4 fragmentation into a proper
>library")
>Cc: mailto:sta...@dpdk.org
>
>Signed-off-by: ...
>
>> I searched for the frag keyword and found no bugs related to this patch.
>I think there is none, but you can create one if you'd like.
>Then, in the commit body, straight before "Fixes: ..." line, don’t forget to
>add:
>Bugzilla ID: <your defect id>
>
>Hope that helps
>Konstantin
>
>From: 蔡慧超 <chcch...@163.com<mailto:chcch...@163.com>>
>Sent: Friday, October 8, 2021 9:06 AM
>To: Ananyev, Konstantin
><konstantin.anan...@intel.com<mailto:konstantin.anan...@intel.com>>
>Cc: cai...@chinatelecom.cn<mailto:cai...@chinatelecom.cn>;
>dev@dpdk.org<mailto:dev@dpdk.org>
>Subject: Re:RE: [PATCH] ip_frag: modify the fragment offset and mf
>
>Hi,Ananyev, Konstantin
>
>Thank you for your reply.I'm sorry for my poor English.This is the first time
>I've submitted a patch to the DPDK, and some of the processes are not
>familiar.I am happy to contribute to the DPDK.
>
>As described in your message：
>
>>As I understand what that patch does - fixes the case when we have
>>to fragment already fragmented ip datagram, correct?
>--Yes，you are right.
>
>>Can I ask you to do few things:
>>1. Reword commit message, it is really misleading right now.
>--Ok, I'll modify the commit message.But I'd like to confirm with you in
>advance how to describe it, because you understand what the patch means.I
>intend to use your explanation as commit information：“Fix the case when we
>have to fragment already fragmented ip datagram.”Is that okay?
>
>> Also if is a fix, then you need to follow:
>>
>> https://doc.dpdk.org/guides/contributing/patches.html#patch-fix-related-issues
>--https://scan.coverity.com/projects/dpdk-data-plane-development-kit
>--I ran into a problem when I clicked the button “View Defects” :
>401: Unauthorized
>Sorry, your credentials are not valid for this resource.
>
>--But now I don't know how to apply for permission, and I'm asking
>mailto:supp...@synopsys.com for help.I don't think this patch should be in
>Coverity.
>
>--Bugzilla
>--I searched for the frag keyword and found no bugs related to this patch.
>>2. Add new test-case for it into app/test/test_ipfrag.c
>--Okay, I'll try to add it.
>
>Best regards.
>huichao cai(Kevin).
>
>
>
>
>
>
>
>At 2021-10-08 01:26:17, "Ananyev, Konstantin"
><mailto:konstantin.anan...@intel.com> wrote:
>>
>>
>>> From: huichao cai <mailto:chcch...@163.com>
>>>
>>> According to RFC791,the fragment offset value should be
>>> calculated based on the long datagram,the more fragments flag
>>> for the last fragment carries the same value as the long datagram.
>>
>>Have to admit, that commit log is really cryptic.
>>I couldn't figure out what it is about till I read the actual code.
>>As I understand what that patch does - fixes the case when we have
>>to fragment already fragmented ip datagram, correct?
>>The code changes itself look ok to me.
>>Can I ask you to do few things:
>>1. Reword commit message, it is really misleading right now.
>> Also if is a fix, then you need to follow:
>>
>> https://doc.dpdk.org/guides/contributing/patches.html#patch-fix-related-issues
>>2. Add new test-case for it into app/test/test_ipfrag.c
>>
>>Thanks
>>
>>>
>>> Signed-off-by: huichao cai <mailto:chcch...@163.com>
>>> ---
>>> lib/ip_frag/rte_ipv4_fragmentation.c | 9 ++++++---
>>> 1 file changed, 6 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/lib/ip_frag/rte_ipv4_fragmentation.c
>>> b/lib/ip_frag/rte_ipv4_fragmentation.c
>>> index 2e7739d..fead5a9 100644
>>> --- a/lib/ip_frag/rte_ipv4_fragmentation.c
>>> +++ b/lib/ip_frag/rte_ipv4_fragmentation.c
>>> @@ -75,7 +75,7 @@ static inline void __free_fragments(struct rte_mbuf
>>> *mb[], uint32_t num)
>>> uint32_t out_pkt_pos, in_seg_data_pos;
>>> uint32_t more_in_segs;
>>> uint16_t fragment_offset, flag_offset, frag_size, header_len;
>>> - uint16_t frag_bytes_remaining;
>>> + uint16_t frag_bytes_remaining, not_last_frag;
>>>
>>> /*
>>> * Formal parameter checking.
>>> @@ -116,7 +116,9 @@ static inline void __free_fragments(struct rte_mbuf
>>> *mb[], uint32_t num)
>>> in_seg = pkt_in;
>>> in_seg_data_pos = header_len;
>>> out_pkt_pos = 0;
>>> - fragment_offset = 0;
>>> + fragment_offset = (uint16_t)((flag_offset &
>>> + RTE_IPV4_HDR_OFFSET_MASK) << RTE_IPV4_HDR_FO_SHIFT);
>>> + not_last_frag = (uint16_t)(flag_offset & IPV4_HDR_MF_MASK);
>>>
>>> more_in_segs = 1;
>>> while (likely(more_in_segs)) {
>>> @@ -186,7 +188,8 @@ static inline void __free_fragments(struct rte_mbuf
>>> *mb[], uint32_t num)
>>>
>>> __fill_ipv4hdr_frag(out_hdr, in_hdr, header_len,
>>> (uint16_t)out_pkt->pkt_len,
>>> - flag_offset, fragment_offset, more_in_segs);
>>> + flag_offset, fragment_offset,
>>> + not_last_frag || more_in_segs);
>>>
>>> fragment_offset = (uint16_t)(fragment_offset +
>>> out_pkt->pkt_len - header_len);
>>> --
>>> 1.8.3.1
>
>
