drivers/block/ub.c: use list_for_each_entry()

drivers/block/ub.c static void ub_reset_task(struct work_struct *work) · line 1611

Quoted 11 lines

drivers/block/ub.c +1 -2

@@ -1608,8 +1605,7 @@ static void ub_reset_task(struct work_struct *work)
 	spin_lock_irqsave(sc->lock, flags);
 	sc->reset = 0;
 	tasklet_schedule(&sc->tasklet);
-	list_for_each(p, &sc->luns) {
-		lun = list_entry(p, struct ub_lun, link);
+	list_for_each_entry(lun, &sc->luns, link) {
 		blk_start_queue(lun->disk->queue);
 	}
 	wake_up(&sc->reset_wait);

This patch straddles the border of acceptable. The pointless obfuscation is balanced against the removal of explicit type in list_entry() and thus a possible mismatched struct. I'm not acking nor naking this.

drivers/block/ub.c static void ub_reset_task(struct work_struct *work) · line 1611

Quoted 10 lines · Matthias Kaehlcke

drivers/block/ub.c +1 -2

@@ -1608,8 +1605,7 @@ static void ub_reset_task(struct work_struct *work)
      spin_lock_irqsave(sc->lock, flags);
      sc->reset = 0;
      tasklet_schedule(&sc->tasklet);
-     list_for_each(p, &sc->luns) {
-             lun = list_entry(p, struct ub_lun, link);
+     list_for_each_entry(lun, &sc->luns, link) {
              blk_start_queue(lun->disk->queue);
      }
      wake_up(&sc->reset_wait);

This patch straddles the border of acceptable. The pointless obfuscation is balanced against the removal of explicit type in list_entry() and thus a possible mismatched struct. I'm not acking nor naking this.

Quoted 7 lines

drivers/block/ub.c static void ub_reset_enter(struct ub_dev *sc, int try) · line 1551

Quoted 3 lines

drivers/block/ub.c +1 -2

-     list_for_each(p, &sc->luns) {
-             lun = list_entry(p, struct ub_lun, link);
+     list_for_each_entry(lun, &sc->luns, link) {

This patch straddles the border of acceptable. The pointless obfuscation is balanced against the removal of explicit type in list_entry() and thus a possible mismatched struct. I'm not acking nor naking this.

A list_for_each() followed by list_entry() ---> list_for_each_entry() conversion is a pretty harmless (and desirable) conversion, IMO. In fact list_for_each_entry() itself uses list_entry(..., typeof(*p), ...) which seems to be a safer way to use list_entry() than specifying the type explicity/manually in its arguments, no?

The negative is the sheer number of helper functions in list.h. Personally, I find it difficult to retain a working knowledge of them. Iterators are particularly nasty that way. I'm thinking about dropping all of these list_for_each_with_murky_argument_requirements_and_odd_side_effects() and use plain for(;;), as a courtesy to someone who has to read the code years down the road.

Quoted 6 lines

The negative is the sheer number of helper functions in list.h. Personally, I find it difficult to retain a working knowledge of them. Iterators are particularly nasty that way. I'm thinking about dropping all of these list_for_each_with_murky_argument_requirements_and_odd_side_effects() and use plain for(;;), as a courtesy to someone who has to read the code years down the road.

I think I disagree with this reasoning. If I'm reading your code and I see, say, list_for_each_entry_safe(), I can be pretty confident that your loop works correctly. If you write your own for loop, then I have to check that you actually got the linked list walking right.

Quoted 11 lines

Quoted 6 lines

The negative is the sheer number of helper functions in list.h. Personally, I find it difficult to retain a working knowledge of them. Iterators are particularly nasty that way. I'm thinking about dropping all of these list_for_each_with_murky_argument_requirements_and_odd_side_effects() and use plain for(;;), as a courtesy to someone who has to read the code years down the road.

I think I disagree with this reasoning. If I'm reading your code and I see, say, list_for_each_entry_safe(), I can be pretty confident that your loop works correctly. If you write your own for loop, then I have to check that you actually got the linked list walking right.

You have to check that I used list_for_each_entry_safe correctly too, which is harder. Are you aware that we had (and probably still have) dozens of cases where the use of list_for_each_entry_safe was buggy? Most of them involved IHV programmers being lured into false sense of security by the _safe suffix and getting their locking wrong.

You could not find a better way to blow up your own argument than to mention list_for_each_entry_safe(), which is anything but. Matthias' use of list_for_each_entry() actually IS safe, which is why I am not NAKing it. Andrew has accepted it already. I just think we aren't winning squat here.

If I just see

	for (pos = list_entry((head)->next, typeof(*pos), member),
		n = list_entry(pos->member.next, typeof(*pos), member);
	     &pos->member != (head);
	     pos = n, n = list_entry(n->member.next, typeof(*n), member))

then what am I to think?

Quoted 7 lines

If I just see

	for (pos = list_entry((head)->next, typeof(*pos), member),
		n = list_entry(pos->member.next, typeof(*pos), member);
	     &pos->member != (head);
	     pos = n, n = list_entry(n->member.next, typeof(*n), member))

Quoted 1 line

then what am I to think?

You won't catch me writing this kind of crap, so the question is moot. Seriously, a comma operator? Admit it, you just expanded a marcro from list.h by hand. Real people cannot write like that.