On Monday 01 March 2010 18:03:49, Matthew Wilcox wrote:
> On Mon, Mar 01, 2010 at 04:15:10PM +0100, Jon Severinsson wrote:
>> Firstly, please note that I'm new to the kernel community, so if I'm
>> doing something wrong, just please politely point it out to me and I'll
>> try to fix it, or at the very least not do it again. Also please bear in
>> mind that English isn't my native language.
> 
> Okey dokey, here's some minor mistakes you made.

Thank you.

>  - Generally a good idea to cc the CIFS development list for CIFS patches.
>    I'm sure they'll notice it here, but just to be on the safe side, I've
>    added them to the cc.

Sure thing. I used linux-fsdevel as I didn't find anything more specific on 
vger.kernel.org, didn't think to look at lists.samba.org.

>  - You've included ifdefs around the check_acl entry in inode_operations,
>    *and* inside the definition of cifs_check_acl.  You only need to do one
>    or the other, and opinion is divided on which is better.

While I did recognize the redundancy, I decided to follow the same convention 
the other functions in xattr.c did, and include ifdefs at both locations.

I also considered the possible reasons for the existing functions to do both, 
and and came up with two reasons. The first simply being the paradigm of 
defensive programming, always check before doing a call, but never assume that 
the check has been done before being called. The second one is that of 
performance. The ifdefs has to be in cifs_check_acl to protect against other 
callers (while this patch doesn't introduce any, it doesn't prevent further 
patches from adding them), and not including the ifdefs in inode_operations 
would mean a completely useless function call on every permission check when a 
feature was turned off at compile time. The second one is a micro-optimization 
I 
don't really care fore, but defensive programming I do respect.

With this in mind, what do you recommend, double protection, breaking 
convention or changing the existing code?

>> --- a/fs/cifs/xattr.c
>> +++ b/fs/cifs/xattr.c
>> @@ -374,3 +374,74 @@ ssize_t cifs_listxattr(struct dentry *direntry, char
>> *data, size_t buf_size) #endif
>>  	return rc;
>>  }
>> +
>> +int cifs_check_acl(struct inode *inode, int mask)
>> +{
>> +	int rc = -EOPNOTSUPP;
>> +#ifdef CONFIG_CIFS_XATTR
>> +#ifdef CONFIG_CIFS_POSIX
>> +	struct dentry *dentry = NULL;
>> +	size_t buf_size;
>> +	void *ea_value = NULL;
>> +	ssize_t ea_size;
>> +	struct posix_acl *acl = NULL;
> 
> I don't think you need to initialise ea_value, do you?

While currently correct, I find it a good idea to immediately null any pointer 
that is freed in the exit section. Otherwise it is very easy to forget to do 
that the day someone adds a goto prior to the first assignment, and not nulling 
then can have unintended consequences in unrelated code. That being said, if 
you say that the kernel community frowns upon that kind of defensive 
programming I will definitely remove it.

With your suggested improvement below, there is no need to NULL dentry though, 
so I'll remove that.

>> +	/* CIFS gets acl from server by path, and thus needs a dentry rather 
>> than 
>> +	   an inode. Note that the path of each dentry will point to the
>> same inode 
>> +	   on the backing fs at the server, so their acls will be
>> the same, and it 
>> +	   doesn't matter which one we pick, so just pick the
>> fist. */ 
>> +	if (list_empty(&inode->i_dentry))
>> +		dentry = list_first_entry(&inode->i_dentry, struct dentry, 
>> d_alias);
>> +
>> +	/* If we didn't get an dentry for the inode, something went terribly
>> wrong. 
>> +	   All we can do at this point is to return an error. */
>> +	if (!dentry || IS_ERR(dentry))
>> +		return PTR_ERR(dentry);
> 
> I don't think dentry can possibly be IS_ERR here, and returning
> PTR_ERR(NULL) doesn't do what you think it does.  Also, you've got the
>  sense of list_empty backwards.  I think what you meant to do here was:
> 
> 	if (!list_empty(&inode->i_dentry))
> 		dentry = list_first_entry(&inode->i_dentry, struct dentry,
> 								d_alias);
> 	else
> 		return -EINVAL;

Sure, this makes more sense. Originally I used d_find_alias(inode), but later 
thought it was better to use the information in the inode struct directly. 
I never really understood how list.h lists was supposed to work, but stopped 
when cut-and-paste got it working for me. Probably a bad idea...

Attaching an updated patch, mostly for the convenience of anyone only 
following linux-cifs-client.

And thanks again for your input.

Best Regards
Jon Severinsson