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? When I send patch,what is the parameters of --subject-prefix?(Which branch(es)?) I don't need to send patches to dev@dpdk.org anymore，right? >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> 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> >Sent: Friday, October 8, 2021 9:06 AM >To: Ananyev, Konstantin <konstantin.anan...@intel.com> >Cc: cai...@chinatelecom.cn; 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 > >
