Josef “Jeff” Sipek

You can look at this post as either a sequal to a post I made few days ago, or as another sequal to a post I made about a year ago.

So, while doing some more cleanup, Dave Quigley discovered this gem in Unionfs:

for (bindex = bstart - 1; bindex >= 0; bindex--) {
        err = copyup_file(dentry->d_parent->d_inode,
                          file, bstart, bindex,
                          file->f_dentry->d_inode->i_size);

        if (!err)
                break;
        else
                continue;
}

The particular piece of interest is the if statement:

if (!err)
        break;
else
        continue;

What does it mean? If there is no error, we break out of the loop. Makes sense. If there is an error, we go to the top of the loop. Makes sense…right? Well, what happens if you reach the end of a for loop? We go to the top of the loop! See the brain damage? We are at the end of the loop, so we don’t need an explicit continue statement.

Here’s the if statement the way it should be (for completeness):

if (!err)
        break;

Oh, so simple, it almost brings a tear into one’s eye.

Think of this as a sequel to my post about a year ago.

The following peice of code is from Unionfs’s copyup_permission(). It is (or should be) a simple function that is supposed to copy the permissions from one inode to another. Well, I can’t help but remember this one slide from Greg Kroah-Hartman’s keynote this year at Ottawa Linux Symposium. Here’s the slide:

The quote definitely applies to copyup_permission(). It seems very clear that it “evolved” to something very odd. Anyway, I shall torment you no longer, here is the code:

static int copyup_permissions(struct super_block *sb,
                              struct dentry *old_hidden_dentry,
                              struct dentry *new_hidden_dentry)
{
        struct iattr newattrs;
        int err;

        print_entry_location();

        newattrs.ia_atime = old_hidden_dentry->d_inode->i_atime;
        newattrs.ia_mtime = old_hidden_dentry->d_inode->i_mtime;
        newattrs.ia_ctime = old_hidden_dentry->d_inode->i_ctime;
        newattrs.ia_valid = ATTR_CTIME | ATTR_ATIME | ATTR_MTIME |
                        ATTR_ATIME_SET | ATTR_MTIME_SET;
        /* original mode of old file */
        newattrs.ia_mode = old_hidden_dentry->d_inode->i_mode;
        newattrs.ia_gid = old_hidden_dentry->d_inode->i_gid;
        newattrs.ia_uid = old_hidden_dentry->d_inode->i_uid;
        newattrs.ia_valid |= ATTR_FORCE | ATTR_GID | ATTR_UID | ATTR_MODE;
        if (newattrs.ia_valid & ATTR_MODE) {
                newattrs.ia_mode = (newattrs.ia_mode & S_IALLUGO) |                
                        (old_hidden_dentry->d_inode->i_mode & ~S_IALLUGO);
        }

        err = notify_change(new_hidden_dentry, &newattrs);

        print_exit_status(err);
        return err;
}

It was actually Seth Arnold that noticed that the condition will ALWAYS be true because ATTR_MODE is set in the line just above it. Furthermore, if one eliminates the if statement and replaces newattr.ia_mode in the assignment with what it is set to just few lines before (right after the comment) he gets:

newattrs.ia_mode = (old_hidden_dentry->d_inode->i_mode & S_IALLUGO) |
                   (old_hidden_dentry->d_inode->i_mode & ~S_IALLUGO);

If you are up to speed with bitwise operations, you’ll realize that it can be simplified to:

newattrs.ia_mode = old_hidden_dentry->d_inode->i_mode;

Brilliant!

So, I was quietly coding, minding my own business, when this crazy guy send me a message:

Crazy Guy: awwww yeah
Crazy Guy: -rw-r–r– 1 sean somegroup 4476447 Apr 11 15:51 tags

Apparently he decided to run ctags on the GCC source code. I couldn’t help but answer:

Jeff: that’s nothing

I ran “make tags” on the 2.6.16 kernel tree, and quickly replied with:

Jeff: -rw-r–r– 1 jsipek somegroup 50423236 Apr 11 15:54 tags

That’s right, the kernel tags file is 11.26 times larger! Or approximately 50% of the size of the GCC source tree! Mwhahaha. He underestimated the power of the kernel. He then went on to say:

Crazy Guy: ctags is my God
Jeff: ok..
Crazy Guy: I’m serious
Crazy Guy: I’m going to set up a little exuberant shrine
Crazy Guy: with a holy ] key suspended from a golden chain
Crazy Guy: and I will sacrifice source files to it every day