On 09/06/2012 04:55 PM, Josh Triplett wrote:

On Thu, Sep 06, 2012 at 03:53:58PM +0200, Sasha Levin wrote:

quoted

On 09/04/2012 07:01 PM, Mathieu Desnoyers wrote:

quoted

quoted

#define do_for_each_ftrace_rec(pg, rec)                                          \

quoted

        for (pg = ftrace_pages_start, rec = &pg->records[pg->index];             \
             pg && rec == &pg->records[pg->index];                               \
             pg = pg->next)                                                      \
          for (rec = pg->records; rec < &pg->records[pg->index]; rec++)

Maybe in some cases there might be ways to combine the two loops into
one ? I'm not seeing exactly how to do it for this one, but it should
not be impossible. If the inner loop condition can be moved to the outer
loop, and if we use (blah ? loop1_conf : loop2_cond) to test for
different conditions depending on the context, and do the same for the
3rd argument of the for() loop. The details elude me for now though, so
maybe it's complete non-sense ;)

It might not be that useful for do_for_each_ftrace_rec, but if we can do
it for the hash table iterator, it might be worth it.

So I think that for the hash iterator it might actually be simpler.

My solution to making 'break' work in the iterator is:A code like that doesn

	for (bkt = 0, node = NULL; bkt < HASH_SIZE(name) && node == NULL; bkt++)
		hlist_for_each_entry(obj, node, &name[bkt], member)

We initialize our node loop cursor with NULL in the external loop, and the
external loop will have a new condition to loop while that cursor is NULL.

My logic is that we can only 'break' when we are iterating over an object in the
internal loop. If we're iterating over an object in that loop then 'node != NULL'.

This way, if we broke from within the internal loop, the external loop will see
node as not NULL, and so it will stop looping itself. On the other hand, if the
internal loop has actually ended, then node will be NULL, and the outer loop
will keep running.

Is there anything I've missed?

Looks reasonable.  However, it would break (or rather, not break) on
code like this:

	hash_for_each_entry(...) {
		if (...) {
			foo(node);
			node = NULL;
			break;
		}
	}

Hiding the double loop still seems error-prone.

I think that that code doesn't make sense. The users of hlist_for_each_* aren't
supposed to be changing the loop cursor.

We have three options here:

 1. Stuff everything into a single for(). While not too difficult, it will make
the readability of the code difficult as it will force us to abandon using
hlist_for_each_* macros.

 2. Over-complicate everything, and check for 'node == NULL && obj &&
obj->member.next == NULL' instead. That one will fail only if the user has
specifically set the object as the last object in the list and the node as NULL.

 3. Use 2 loops which might not work properly if the user does something odd,
with a big fat warning above them.


To sum it up, I'd rather go with 3 and let anyone who does things he shouldn't
be doing break.


Thanks,
Sasha