Thoughts on the ext4 panic [LWN.net]

Thoughts on the ext4 panic

Posted Oct 30, 2012 9:54 UTC (Tue) by niner (subscriber, #26151)
In reply to: Thoughts on the ext4 panic by nix
Parent article: Thoughts on the ext4 panic

The solution to the horrible formatting of do_one_pass() is not to increase the character limit, but to factor out parts of the function to decrease indentation levels. Some parts are indented 8 levels.

To quote CodingStyle: "The answer to that is that if you need more than 3 levels of indentation, you're screwed anyway, and should fix your program."

do_one_pass() is 390 lines long making it very hard to see it's structure anyway which just strengthens the indication of needed refactoring.

(Log in to post comments)

Thoughts on the ext4 panic

Posted Oct 30, 2012 10:43 UTC (Tue) by nix (subscriber, #2304) [Link]

So... you think that if you split each case statement into its own function, it would somehow become better designed?

Sorry, I'm as much a fan of functional decomposition as the next man -- actually I go a bit nuts about it -- but the key to that is splitting functions at logical boundaries. Splitting at random because the indentation passes some arbitrary 'too deep' level, when you would not have split it otherwise, is ridiculous -- and this function indicates that the kernel developers do not in fact do this ridiculous thing. (Obviously there *is* an effective length/complexity limit, and perhaps do_one_pass() is past it -- but a hypothetical function with a long stretch of low indentation with a bit of high indentation in the middle of it is *not* made any clearer by splitting the high indentation into a completely arbitrary separate function!)

Thoughts on the ext4 panic

Posted Oct 30, 2012 11:00 UTC (Tue) by niner (subscriber, #26151) [Link]

It may become better designed or maybe not. I did not read the code in detail because of too little time to decifer the code.

Maybe I was a bit unclear on this: if there is indeed not a single logical boundary in the function, then well, tough luck. Then this is one of the rare cases where there's just no good solution. Even with an increased character limit, the function would not be that much more readable. It would still be too long to get a good grasp of the structure quickly.

I did not mean that arbitrarily cutting up the function would be a solution, just that usually it is very much possible to decompose and achieve increased readability. That's why I used the word "indication" in my last sentence.

Thoughts on the ext4 panic

Posted Oct 30, 2012 14:14 UTC (Tue) by viro (subscriber, #7872) [Link]

oh, give me a break...

tagp = pointer; // fairly fat expression, actually
while (tagp - pointer <= size) { // again, and so's 'size'
tag = (journal_block_tag_t *) tagp;
flags = be32_to_cpu(tag->t_flags);
/* couple of lines */
err = jread(&obh, journal, io_block);
if (err) {
success = err;
printk(....);
} else {
/* about 50 lines */
}
skip_write:
tagp += sizeof(journal_block_tag_t);
if (!(flags & JFS_FLAG_SAME_UUID))
tagp += 16;

if (flags & JFS_FLAG_LAST_TAG)
break;
}

In a case. Wrapped in a while. Only at Dibbler's, and that's cutting me own throat...

Seriously, in this case 80-column heuristic has worked nicely - the code structure is badly obfuscated and 4-character tabs would only hide a visible indicator of trouble.