Re: [PATCH] cifs: add comments explaining cifs_new_fileinfo behavior

On 05/11/2010 04:17 PM, Jeff Layton wrote:
> On Mon, 10 May 2010 20:00:05 +0530
> Suresh Jayaraman <[email protected]> wrote:
> 
>> The comments make it clear the otherwise subtle behavior of cifs_new_fileinfo().
>>
>> Signed-off-by: Suresh Jayaraman <[email protected]>
>> --
>>  fs/cifs/dir.c |   18 ++++++++++++++++--
>>  1 files changed, 16 insertions(+), 2 deletions(-)
>>
>> diff --git a/fs/cifs/dir.c b/fs/cifs/dir.c
>> index d791d07..bd363df 100644
>> --- a/fs/cifs/dir.c
>> +++ b/fs/cifs/dir.c
>> @@ -129,6 +129,12 @@ cifs_bp_rename_retry:
>>  	return full_path;
>>  }
>>  
>> +/*
>> + * When called with struct file pointer set to NULL, there is no way we could
>> + * update file->private_data, but getting it stuck on openFileList provides a
>> + * way to access it from cifs_fill_filedata and thereby set file->private_data
>> + * from cifs_open.
>> + */

> 
> The comments help, but this code has bothered me for some time. Is it
> possible for the create to return success and for something else to
> happen such that the cifs_open is never called? I'd imagine that it is,
> and if so, then this this open file will be "leaked".

I asked Shirish exactly the same question while discussing in the
#samba-technical irc channel. He does not see a leak, but thought you or
Steve will have a better idea..

Steve:  is such a situation not possible at all?

> I think we should be using lookup_instantiate_filp here instead and
> doing this similarly to the open on lookup code. There is some
> precedent for this -- fuse_create_open does this and it gets called via
> the .create operation.
> 

I thought about this briefly but was not sure whether it can be called
from cifs_create. I'll need to dig more.


Thanks,

-- 
Suresh Jayaraman