Re: [BUGFIX][PATCH 3/3] configfs: Fix failing symlink() making rmdir() fail

Joel Becker
2008-06-19T22:22:34Z

On Tue, Jun 17, 2008 at 07:37:23PM +0200, Louis Rilling wrote:

Ok, I can see some of why I hated this.

> +
> spin_lock(&configfs_dirent_lock);
> - if (target_sd->s_type & CONFIGFS_USET_DROPPING) {
> - spin_unlock(&configfs_dirent_lock);
> - config_item_put(item);
> - kfree(sl);
> - return -EPERM;
> - }
> - list_add(&sl->sl_list, &target_sd->s_links);
> + /*
> + * Force rmdir() of parent_item to wait until we know if we
> + * succeed.
> + */
> + parent_sd->s_type |= CONFIGFS_USET_ATTACHING;
> spin_unlock(&configfs_dirent_lock);
> +
> ret = configfs_create_link(sl, parent_item->ci_dentry,
> dentry);
> - if (ret) {
> - spin_lock(&configfs_dirent_lock);
> - list_del_init(&sl->sl_list);
> +
> + spin_lock(&configfs_dirent_lock);
> + parent_sd->s_type &= ~CONFIGFS_USET_ATTACHING;
> +
> + if (ret || target_sd->s_type & CONFIGFS_USET_DROPPING) {

Use parenthesis. Actually, separate out the error cases.

> + struct configfs_dirent *sd = NULL;
> +
> + if (!ret) {
> + sd = dentry->d_fsdata;
> + list_del_init(&sd->s_sibling);
> + }
> spin_unlock(&configfs_dirent_lock);
> +
> + if (!ret) {
> + configfs_drop_dentry(sd, dentry->d_parent);
> + d_delete(dentry);
> + configfs_put(sd);
> + }

This open-code of the VFS munging is ripe to break when the VFS
changes or other configfs changes happen. The real issue is that you
are reimplementing the core of configfs_unlink(). Note how the core VFS
munging of configfs_rmdir() is separated out so that configfs_mkdir()
can also use it in the failure case? Do the same with unlink() and it
will read much better ("if (DROPPING) configfs_delete_link()"). Call it
configfs_remove_link() or configfs_delete_link().

Joel

--

Life's Little Instruction Book #337

"Reread your favorite book."

Joel Becker
Principal Software Developer
Oracle
E-mail: joel.becker@oracle.com
Phone: (650) 506-8127