On Thu, 2009-01-01 at 12:12 -0500, Peter Memishian wrote: > > ip_multi.c: > > > > 1950: In the case where ill is an underlying interface, we're calling > > ill_ilm_walker_hold() on ill, but not on ilw_ipmp_ill (the IPMP > > interface). Can that lead to ilm's on the IPMP interface returned by > > the ilm_walker_*() being manipulated or deleted by other threads while > > referenced by the caller? > > If we're called with an underlying interface, then we will always try to > set ilw_ipmp_ill via ipmp_ill_hold_ipmp_ill(). That call *could* fail if > e.g. the IPMP ill was changing or somesuch. In that case though, we > won't walk to the IPMP ill because ilw_ipmp_ill wil be NULL, and therefore > we won't set ilw_walk_ill() to it. In other words, the code is written > such that we always acquire holds on all of the ills that we will walk > over (but there could be a bug.)
My concern wasn't that we're not holding a reference to the ill (I see that ipmp_ill_hold_ipmp_ill() does that), but rather we're not bumping ill_ilm_walker_cnt for the IPMP interface. Is that not required? > > 1642: This function's implementation is a bit twisted given the comment > > above it that states that it's only used for IPv4. If that's the case, > > then why go through the trouble of converting things to IPv6 addresses > > to compare against ilm_v6addr instead of just comparing against > > ilm_addr? Anyway, that's not your doing, it was like that before. > > It does seem more difficult than necessary. I'll change it. Great. > > 1966: ilm_walker_step_helper() is only used by ilm_walker_step(), so it > > should be static. Perhaps ilm_walker_step_all() would be more > > descriptive (just a suggestion). > > Sure, I'll make those changes. Okay, thanks. -Seb
