| Message ID | 152210718418.11435.11573013181393548255.stgit@noble |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | David Miller |
| Headers | show |
| Series | rhashtable: assorted fixes and enhancements | expand |
Hello! On 3/27/2018 2:33 AM, NeilBrown wrote: > The documentation for rhashtable_walk_peek() wrong. It claims to > return the *next* entry, whereas it in fact returns the *previous* > entry. > However if no entries have yet been returned - or if the iterator > was reset due to a resize event, then rhashtable_walk_peek() > *does* return the next entry, but also advances the iterator. > > I suspect that this interface should be discarded and the one user > should be changed to not require it. Possibly this patch should be > seen as a first step in that conversation. > > This patch mostly corrects the documentation, but does make a > small code change so that the documentation can be correct without > listing too many special cases. I don't think the one user will > be affected by the code change. > > Signed-off-by: NeilBrown <neilb@suse.com> > --- > lib/rhashtable.c | 17 +++++++++++++---- > 1 file changed, 13 insertions(+), 4 deletions(-) > > diff --git a/lib/rhashtable.c b/lib/rhashtable.c > index 3825c30aaa36..24a57ca494cb 100644 > --- a/lib/rhashtable.c > +++ b/lib/rhashtable.c > @@ -853,13 +853,17 @@ void *rhashtable_walk_next(struct rhashtable_iter *iter) > EXPORT_SYMBOL_GPL(rhashtable_walk_next); > > /** > - * rhashtable_walk_peek - Return the next object but don't advance the iterator > + * rhashtable_walk_peek - Return the previously returned object without advancing the iterator > * @iter: Hash table iterator > * > - * Returns the next object or NULL when the end of the table is reached. > + * Returns the last object returned, Sounds somewhat tautological. :-) > or NULL if no object has yet been returned. > + * If the previously returned object has since been removed, then some other arbitrary > + * object maybe returned, or possibly NULL will be returned. In that case, the > + * iterator might be advanced. > * > * Returns -EAGAIN if resize event occurred. Note that the iterator > - * will rewind back to the beginning and you may continue to use it. > + * will rewind back to the beginning and rhashtable_walk_next() should be > + * used to get the next object. > */ > void *rhashtable_walk_peek(struct rhashtable_iter *iter) > { > @@ -880,7 +884,12 @@ void *rhashtable_walk_peek(struct rhashtable_iter *iter) > * the table hasn't changed. > */ > iter->skip--; > - } > + } else > + /* ->skip is only zero after rhashtable_walk_start() > + * or when the iterator is reset. In this case there > + * is no previous object to return. > + */ > + return NULL; CodingStyle: need {} on the *else* branch if the 1st branch has them. > > return __rhashtable_walk_find_next(iter); > } MBR, Sergei
On Tue, Mar 27, 2018 at 10:33:04AM +1100, NeilBrown wrote: > The documentation for rhashtable_walk_peek() wrong. It claims to > return the *next* entry, whereas it in fact returns the *previous* > entry. > However if no entries have yet been returned - or if the iterator > was reset due to a resize event, then rhashtable_walk_peek() > *does* return the next entry, but also advances the iterator. > > I suspect that this interface should be discarded and the one user > should be changed to not require it. Possibly this patch should be > seen as a first step in that conversation. > > This patch mostly corrects the documentation, but does make a > small code change so that the documentation can be correct without > listing too many special cases. I don't think the one user will > be affected by the code change. > > Signed-off-by: NeilBrown <neilb@suse.com> We should cc Tom Herbert too since he wrote this code. > --- > lib/rhashtable.c | 17 +++++++++++++---- > 1 file changed, 13 insertions(+), 4 deletions(-) > > diff --git a/lib/rhashtable.c b/lib/rhashtable.c > index 3825c30aaa36..24a57ca494cb 100644 > --- a/lib/rhashtable.c > +++ b/lib/rhashtable.c > @@ -853,13 +853,17 @@ void *rhashtable_walk_next(struct rhashtable_iter *iter) > EXPORT_SYMBOL_GPL(rhashtable_walk_next); > > /** > - * rhashtable_walk_peek - Return the next object but don't advance the iterator > + * rhashtable_walk_peek - Return the previously returned object without advancing the iterator > * @iter: Hash table iterator > * > - * Returns the next object or NULL when the end of the table is reached. > + * Returns the last object returned, or NULL if no object has yet been returned. > + * If the previously returned object has since been removed, then some other arbitrary > + * object maybe returned, or possibly NULL will be returned. In that case, the > + * iterator might be advanced. > * > * Returns -EAGAIN if resize event occurred. Note that the iterator > - * will rewind back to the beginning and you may continue to use it. > + * will rewind back to the beginning and rhashtable_walk_next() should be > + * used to get the next object. > */ > void *rhashtable_walk_peek(struct rhashtable_iter *iter) > { > @@ -880,7 +884,12 @@ void *rhashtable_walk_peek(struct rhashtable_iter *iter) > * the table hasn't changed. > */ > iter->skip--; > - } > + } else > + /* ->skip is only zero after rhashtable_walk_start() > + * or when the iterator is reset. In this case there > + * is no previous object to return. > + */ > + return NULL; > > return __rhashtable_walk_find_next(iter); > } > >
From: NeilBrown <neilb@suse.com> Date: Tue, 27 Mar 2018 10:33:04 +1100 > The documentation for rhashtable_walk_peek() wrong. It claims to > return the *next* entry, whereas it in fact returns the *previous* > entry. > However if no entries have yet been returned - or if the iterator > was reset due to a resize event, then rhashtable_walk_peek() > *does* return the next entry, but also advances the iterator. > > I suspect that this interface should be discarded and the one user > should be changed to not require it. Possibly this patch should be > seen as a first step in that conversation. > > This patch mostly corrects the documentation, but does make a > small code change so that the documentation can be correct without > listing too many special cases. I don't think the one user will > be affected by the code change. > > Signed-off-by: NeilBrown <neilb@suse.com> Please mention the "one user" explicitly in both locations where you refer to it in this commit message. Thank you.
Neil, 2018-03-27 1:33 GMT+02:00 NeilBrown <neilb@suse.com>: > The documentation for rhashtable_walk_peek() wrong. It claims to > return the *next* entry, whereas it in fact returns the *previous* > entry. > However if no entries have yet been returned - or if the iterator > was reset due to a resize event, then rhashtable_walk_peek() > *does* return the next entry, but also advances the iterator. > > I suspect that this interface should be discarded and the one user > should be changed to not require it. Possibly this patch should be > seen as a first step in that conversation. > > This patch mostly corrects the documentation, but does make a > small code change so that the documentation can be correct without > listing too many special cases. I don't think the one user will > be affected by the code change. how about I come up with a replacement so that we can remove rhashtable_walk_peek straight away without making it differently broken in the meantime? Thanks, Andreas
On Wed, Mar 28 2018, Andreas Grünbacher wrote: > Neil, > > 2018-03-27 1:33 GMT+02:00 NeilBrown <neilb@suse.com>: >> The documentation for rhashtable_walk_peek() wrong. It claims to >> return the *next* entry, whereas it in fact returns the *previous* >> entry. >> However if no entries have yet been returned - or if the iterator >> was reset due to a resize event, then rhashtable_walk_peek() >> *does* return the next entry, but also advances the iterator. >> >> I suspect that this interface should be discarded and the one user >> should be changed to not require it. Possibly this patch should be >> seen as a first step in that conversation. >> >> This patch mostly corrects the documentation, but does make a >> small code change so that the documentation can be correct without >> listing too many special cases. I don't think the one user will >> be affected by the code change. > > how about I come up with a replacement so that we can remove > rhashtable_walk_peek straight away without making it differently > broken in the meantime? > Hi Andreas, I'd be very happy with that outcome - thanks for the offer! NeilBrown
diff --git a/lib/rhashtable.c b/lib/rhashtable.c index 3825c30aaa36..24a57ca494cb 100644 --- a/lib/rhashtable.c +++ b/lib/rhashtable.c @@ -853,13 +853,17 @@ void *rhashtable_walk_next(struct rhashtable_iter *iter) EXPORT_SYMBOL_GPL(rhashtable_walk_next); /** - * rhashtable_walk_peek - Return the next object but don't advance the iterator + * rhashtable_walk_peek - Return the previously returned object without advancing the iterator * @iter: Hash table iterator * - * Returns the next object or NULL when the end of the table is reached. + * Returns the last object returned, or NULL if no object has yet been returned. + * If the previously returned object has since been removed, then some other arbitrary + * object maybe returned, or possibly NULL will be returned. In that case, the + * iterator might be advanced. * * Returns -EAGAIN if resize event occurred. Note that the iterator - * will rewind back to the beginning and you may continue to use it. + * will rewind back to the beginning and rhashtable_walk_next() should be + * used to get the next object. */ void *rhashtable_walk_peek(struct rhashtable_iter *iter) { @@ -880,7 +884,12 @@ void *rhashtable_walk_peek(struct rhashtable_iter *iter) * the table hasn't changed. */ iter->skip--; - } + } else + /* ->skip is only zero after rhashtable_walk_start() + * or when the iterator is reset. In this case there + * is no previous object to return. + */ + return NULL; return __rhashtable_walk_find_next(iter); }
The documentation for rhashtable_walk_peek() wrong. It claims to return the *next* entry, whereas it in fact returns the *previous* entry. However if no entries have yet been returned - or if the iterator was reset due to a resize event, then rhashtable_walk_peek() *does* return the next entry, but also advances the iterator. I suspect that this interface should be discarded and the one user should be changed to not require it. Possibly this patch should be seen as a first step in that conversation. This patch mostly corrects the documentation, but does make a small code change so that the documentation can be correct without listing too many special cases. I don't think the one user will be affected by the code change. Signed-off-by: NeilBrown <neilb@suse.com> --- lib/rhashtable.c | 17 +++++++++++++---- 1 file changed, 13 insertions(+), 4 deletions(-)