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.
