Re: [PATCH 24/29] sysctl: Replace root_list with links between sysctl_table_sets.

[ebiederm@xmission.com (Eric W. Biederman)]

Lucian Adrian Grijincu <lucian.grijincu@gmail.com> writes:

> Very nice way to handle netns specific files with links between sets!
>
> You did a much better job than I did at dealing with them.
>
> It took me a while to understand how the code works. I'll try to write
> something for Documentation/ because the inner workings are a bit
> intertwined.
>
> A few comments bellow.

Thank you for your review.

I'm going to see how many of your comments I can incorporate into the
code.

Since no significant bugs have been seen my plan is to use incremental
patches on top of the existing code, to address the small issues that
have been seen.  That way I don't have to retest yet again.

>>  void register_sysctl_root(struct ctl_table_root *root)
>>  {
>> -       spin_lock(&sysctl_lock);
>> -       list_add_tail(&root->root_list, &sysctl_table_root.root_list);
>> -       spin_unlock(&sysctl_lock);
>>  }
>
>
> This function is empty. Can be deleted (there are two callers in
> net/sysctl_net.c.

Agreed.  There might be something useful by registering roots.  Perhaps
initialize the default set.  So I have kept the function for the short
term.  register_sysctl_root makes sense as an interface.  We just don't
have anything for register_sysctl_root to do right now.

>> @@ -400,6 +373,7 @@ static struct dentry *proc_sys_lookup(struct inode *dir, struct dentry *dentry,
>>        struct inode *inode;
>>        struct dentry *err = ERR_PTR(-ENOENT);
>>        struct ctl_dir *ctl_dir;
>> +       int ret;
>>
>>        if (IS_ERR(head))
>>                return ERR_CAST(head);
>> @@ -410,6 +384,11 @@ static struct dentry *proc_sys_lookup(struct inode *dir, struct dentry *dentry,
>>        if (!p)
>>                goto out;
>>
>
>
> "sysctl_follow_link" implies that it will follow the link. I would
> pull out the check whether the header is a link or not. This wouldn't
> save much (a function call), but it would make the code easier to
> read:
>
> 	/* Get out quickly if not a link */
>         if (S_ISLNK(p->mode)) {
> 		ret = sysctl_follow_link(&h, &p, current->nsproxy);
> 		err = ERR_PTR(ret);
> 		if (ret)
> 		        goto out;
>         }

Good point, and obviousness is part of what I am aiming for.


>>  {
>> +       struct ctl_table_set *set = dir->header.set;
>>        struct ctl_dir *subdir, *new = NULL;
>>
>>        spin_lock(&sysctl_lock);
>> -       subdir = find_subdir(dir->header.set, dir, name, namelen);
>> -       if (!IS_ERR(subdir))
>> -               goto found;
>> -       if ((PTR_ERR(subdir) == -ENOENT) && set != dir->header.set)
>> -               subdir = find_subdir(set, dir, name, namelen);
>> +       subdir = find_subdir(dir, name, namelen);
>>        if (!IS_ERR(subdir))
>>                goto found;
>>        if (PTR_ERR(subdir) != -ENOENT)
>> @@ -817,13 +815,14 @@ static struct ctl_dir *get_subdir(struct ctl_table_set *set,
>>        if (!new)
>>                goto failed;
>>
>> -       subdir = find_subdir(set, dir, name, namelen);
>> +       subdir = find_subdir(dir, name, namelen);
>>        if (!IS_ERR(subdir))
>>                goto found;
>>        if (PTR_ERR(subdir) != -ENOENT)
>>                goto failed;
>
> I think you're returning the wrong error here. If we got to this point
> then subdir == ERR_PTR(-ENOENT).
> We want to create a new dir here even if one doesn't exist.
> So if we have an error in insert_header() we don't return that error,
> but ENOENT.
>
>>
>> -       insert_header(dir, &new->header);
>> +       if (insert_header(dir, &new->header))
>> +               goto failed;
>>        subdir = new;
>
> Something like this would fix it:
>
>         if (subdir = insert_header(dir, &new->header))
>
>
>
>>  found:
>>        subdir->header.nreg++;
>> @@ -841,6 +840,57 @@ failed:
>>        return subdir;
>>  }
>>
--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html