IfAPI: Add iterator to complement if_foreach()

[jhibbits]

Summary

Sometimes an if_foreach() callback can be trivial, or need a lot of
outer context. In this case a regular for loop makes more sense. To
keep things hidden in the new API, use an opaque if_iter structure
that can still be instantiated on the stack. The current implementation
uses just a single pointer out of the 4 alotted to the opaque context,
and the cleanup does nothing, but may be used in the future.

[melifaro]

Generally LGTM, just want to ensure we're on the same page w.r.t the iteration KPI for the other if* use cases.

sys/net/if.c

4607

I like the compact iteration form it provides.
Do you think it would make sense to provide the same iteration KPI for other similar cases (ifaddrs, ifmaddrs etc)?
What do you think of having the same form for if_foreach_sleep()?

[melifaro]

sys/net/if.c

4613

Nit: just to be safe, I'd suggest handling the deletion case here as well.

[melifaro]

sys/net/if.c

4628

Nit: (sorry, naming is hard, and everyone has opinions). Would it be possible to use a pair of antonym verbs to make it easier to remember? For example, like begin/end in C++ iterators, or start/stop, or something similar?

[jhibbits]

sys/net/if.c

4607

What is the technical difference between if_foreach() and if_foreach_sleep() that this doesn't address? Sorry, I haven't looked closely at if_foreach_sleep().

Is there a good use case for the same kind of KPI for ifaddr, ifmaddr, etc? I think the callback API is sufficient as-is for those in most cases.

4613

Do you mean like your comment above, of saving "cur" and "next", to be an analogue to the STAILQ_FOREACH_SAFE() macro set?

4628

Sure. I like begin() and end(), or start() and finish() (stop() sounds more like abort() than "conclude the iteration"). Maybe start() and finish(), so as not to be confused with C++'s begin() and end() uses?

[melifaro]

sys/net/if.c

4607

Guess it's more about consistency. Iterator KPI is simpler to use (at least from my view) and it'd be nice to have a single common way of doing certain things. E.g. if we have iterators for the interfaces, why having if_foreach() with callbacks?
If we have iterators for the interfaces, why do we have ifa iteration with callbacks and so on? Given the KPI is new, it's not too late to do the change once again (if there's an agreement). I can work on that.

Re if_foreach_sleep() - it allows sleeping when iterating over the list of interfaces. Sleeping makes epoch (or lock) usage impossible, making the implementation a bit more complex.

4613

Yes, exactly. Doesn't cost much, but adds a bit of safety.

4628

I'm fine with start / finish. Also I'm curious why reusing C++ names here is bad? e.g. what undesired side effect[s] you have in mind?

[jhibbits]

sys/net/if.c

4607

Orthogonality makes sense. If you want to work on that, go for it. Given this review, adding the same for others should be trivial.

4628

There is no technical reason, more a social reason. When someone sees foo_begin() and foo_end() coupled with iterators it's easy for someone with a C++ background to think "start and end sentinels" rather than "start the iteration" and "complete the iteration"

[jhibbits]

Address feedback from @melifaro. Make the API 'safe', and change the cleanup function to 'finish'.

[melifaro]

Thank you for addressing the comments! LGTM.

sys/net/if.c

4607

Thank you! I'll come up with the review(s) in a day or two.

4627

Nit: do we actually need it?
Actually, I guess one can just store the 'next' pointer in the iterator.

[jhibbits]

sys/net/if.c

4627

You're right, there's no need to store the current pointer in the iterator, it can just be returned, leaving only next.

[jhibbits]

Simplify things a bit, store only next pointer instead of both current and next.

[glebius]

So what is the plan: eventually substitute if_foreach() with iterator or to have both?

[melifaro]

In D39138#891860, @glebius wrote:

So what is the plan: eventually substitute if_foreach() with iterator or to have both?

I’d love to have iterator-only interfaces (patch WIP)

[melifaro]

I've created D39200 with the remaining iterators, please take a look :-)