> -----Original Message----- > From: David Ahern [mailto:d...@cumulusnetworks.com] > Sent: Monday, November 28, 2016 11:10 AM > To: 张胜举 <zhangshen...@cmss.chinamobile.com>; > netdev@vger.kernel.org > Subject: Re: [net,v2] neigh: fix the loop index error in neigh dump > > On 11/27/16 7:56 PM, David Ahern wrote: > > On 11/27/16 7:53 PM, 张胜举 wrote: > >> > >> > >>> -----Original Message----- > >>> From: David Ahern [mailto:d...@cumulusnetworks.com] > >>> Sent: Monday, November 28, 2016 10:39 AM > >>> To: 张胜举 <zhangshen...@cmss.chinamobile.com>; > >>> netdev@vger.kernel.org > >>> Subject: Re: [net,v2] neigh: fix the loop index error in neigh dump > >>> > >>> On 11/27/16 7:34 PM, 张胜举 wrote: > >>>>> -----Original Message----- > >>>>> From: David Ahern [mailto:d...@cumulusnetworks.com] > >>>>> Sent: Monday, November 28, 2016 10:10 AM > >>>>> To: Zhang Shengju <zhangshen...@cmss.chinamobile.com>; > >>>>> netdev@vger.kernel.org > >>>>> Subject: Re: [net,v2] neigh: fix the loop index error in neigh > >>>>> dump > >>>>> > >>>>> On 11/27/16 6:32 PM, Zhang Shengju wrote: > >>>>>> Loop index in neigh dump function is not updated correctly under > >>>>>> some circumstances, this patch will fix it. > >>>>> > >>>>> What's an example? > >>>> > >>>> If dev is filtered out, the original code goes to next loop without > >>>> updating loop index 'idx'. > >>> > >>> And you have a use case with missing or redundant data? Or is your > >>> comment based on a review of code only? > >> It's on my code review. No use case currently, this is uncommon to > happen. > >> > >> > >>> > >>>>> You are completely rewriting the dump loops. > >>>> > >>>> I put 'idx++' into for loop, so I replace 'goto' with 'continue'. > >>>> The other change is style related. > >>> > >>> A "fixes" should not include 'style related' changes. > >> Okay, I will send another version without style changes. > >> > > > > Personally, I think you need to produce a use case that fails before sending > another patch. I have not seen a problem with this code. > > > > And looking back at 3f0ae05d6f I should not have acked it (reviewed it too > quickly while on PTO). Your change is a no-op because of what idx represents > - the position in the hash list for devices relevant for the dump request. > Same goes for the neigh dump so this patch is not needed. > No, when dump request must be processed by multiple 'recv/recvmsg' system calls, idx stores which dev/neigh the previous call have processed, so that next call will scan from the right place.
So no matter whether the dev/neigh is filtered, the idx should be increased anyway. It's hard to produce a use case, because we mostly have only one entity in hash list. Even with multiple entities, we also need the function to exit right at the place where dev/neigh is filter out. All other dump functiones for RT netlink keep this logic, you can refer inet_dump_ifaddr() if you wish.
