Thoughts on the ext4 panic [LWN.net]

Thoughts on the ext4 panic

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

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!)

(Log in to post comments)

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.